Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Native ES Module support for Oclif w/ continued support of CommonJS #223

Closed
wants to merge 1 commit into from
Closed

Native ES Module support for Oclif w/ continued support of CommonJS #223

wants to merge 1 commit into from

Conversation

typhonrt
Copy link

@typhonrt typhonrt commented Mar 7, 2021

A pre-announcement edit: While I have initially prototyped ES Module support in Oclif v1 please refer to my modifications to the newly announced Oclif v2 which is where ESM support should be added. You can follow adding ESM to Oclif v2 in @oclif/core:
oclif/core#130

I am closing this pull request for Oclif v1.

Original message:

Greets...
Please see the associated Issue for pleasantries.

This pull request updates @oclif/config to support loading of CommonJS & ES Modules (ESM) package plugins for commands and associated hooks. In short a package that has "type": "module" set in package.json is treated as an ESM package and dynamic import / import() will be used to load any commands and hooks from that package. It is also possible for the main CLI code to also be ESM. These changes transparently support loading both CommonJS & ESM so plugins in a CLI can be a combination of both.

The usage of dynamic import is gated by checking that package.json type is set to module and if not set then require() is used like the old code.

The main changes are to config.ts, plugin.ts and the addition of import-dynamic.ts. The latter new source file simply exports a wrapper around import(): export default new Function('modulePath', 'return import(modulePath)'). This is necessary as tsc when set for "module": "commonJS" will transpile import() into require(). I searched for any tsconfig setting that will selectively not transpile import(), but found no configuration based solution. This seems to be the least impactful option and is concise maintaining no changes to tsconfig.json or the build process. I'm certainly open to learning if there is another way to prevent tsc from altering dynamic imports in CommonJS targeted output. It should be noted that dynamic imports / import() is available to CommonJS code to load ESM on versions of Node that support "type": "module" set in package.json.

The largest flow control change was to runHook in config.ts which needs to be restructured to support await import() and require in the same block of code. The previous Promise.all and Array.map code was replaced with synchronous looping with a single await line. This ensures that mixed loading with dynamic import and require executes synchronously. The old Promise.all / Array.map control flow had execution of hooks out of order (essentially reverse completion / require first) when combining dynamic import and require loading due to the nested anonymous async function in .map() with a mixed await import() / no await, require(), situation. I do believe the new runHook code is simpler and captures the intention of loading hooks in order. Each hook starts / finishes before the next one is called. The old code only worked due to the synchronous nature of require() and underlying implementation in Node.

For main core CLI code switching to ESM one adds "type": "module" to package.json and must change ./bin/run to ./bin/run.js so Node can associate it as ESM and add the following to the top of ./bin/run.js:

import { createRequire } from 'module';
const require = createRequire(import.meta.url);

Using createRequire is the easiest way to get the bootstrapping code to function correctly and also provides a simple way to switch to ESM. The rest of a CLI codebase can be converted to normal ESM at this point. Any plugins referenced can be a mixture of CommonJS or ESM.

This is a potential big change and one that is very timely at this point as ESM is increasingly being used on Node. All tests pass and no new tests were added. I tested these changes in my own CLI which I partially converted to ESM while testing, so I was loading a mixture of ESM / CJS plugins before a full conversion to all ESM. It certainly is recommended to test these changes on older versions of Node though the test suite passes on 10.x / 12.x. The usage of dynamic import is only be engaged when package.json indicates it should be used and developers are only setting this for Node versions that support it. I only tested my CLI on Node 14.15.1 on OSX and am releasing my CLI requiring Node 14.x+.

My non-trivial CLI test case is fvttdev which is published in a pre-alpha but working state:
https://github.com/typhonjs-fvtt/fvttdev
https://www.npmjs.com/package/@typhonjs-fvtt/fvttdev

Integration with an actual demo project using the NPM published version:
https://github.com/typhonjs-fvtt/demo-fvttdev-module

I have posted a separate version of my changes to this repo which can be included directly from Github as a replacement to @oclif/config:

https://github.com/typhonjs-oclif/config

For example from fvttdev package.json:
https://github.com/typhonjs-fvtt/fvttdev/blob/main/package.json#L15

"@oclif/config": "git+https://github.com/typhonjs-oclif/config.git"

Anyway I look forward to working with any project maintainers to get this update into Oclif.


I have also made modifications to @oclif/command & @oclif/plugin-help to be able to load custom help classes as an ES Module and have tested it along with making sure the test suite passes. I haven't submitted them as PRs as there is a co-dependency between these two modules so the tests would fail unless both of these modules are updated.

The last concern about updating Oclif to support ES Modules natively regards @oclif/dev-cli and particularly the readme command as it invokes the help class. I would be glad to make the modifications necessary to @oclif/dev-cli when I make contact with any project maintainers regarding the willingness to accept support for ES Modules.

All my forks have a long term home in the typhonjs-oclif-scratch Github organization. I do hope to make contact with project maintainers, but am also preparing for things to take a while.

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #223 (5dd7b52) into master (0779d64) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #223   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           2       3    +1     
  Lines          12      13    +1     
  Branches        3       3           
======================================
- Misses         12      13    +1     
Impacted Files Coverage Δ
src/import-dynamic.ts 0.00% <0.00%> (ø)
src/index.ts 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0779d64...5dd7b52. Read the comment docs.

@typhonrt
Copy link
Author

typhonrt commented Apr 1, 2021

While I have initially prototyped ES Module support in Oclif v1 please refer to my modifications to the newly announced Oclif v2 which is where ESM support should be added. You can follow adding ESM to Oclif v2 in @oclif/core:
oclif/core#130

I am closing this pull request for Oclif v1.

@typhonrt typhonrt closed this Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant