-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES Module support for Oclif v2 #130
Comments
So I have finished the comprehensive test suites demonstrating ESM Oclif CLIs based on my fork of @oclif/core and three variations which provide the widest support to what can be considered modern ESM on Node. The tests are run via Github Actions across a matrix supporting The test suites have detailed information about what limitations they address, so please refer to the README of each:
The "modern" test suite works on Node Proper documentation and example projects can be offered allowing end users to manually configure an ESM CLI for wider ecosystem support addressing two caveats. The first caveat is that until Node The second caveat is that below Node Regarding code coverage of the Mocha ESM tests. nyc presently does not support code coverage for tests written in ESM, but a drop in replacement of c8 handles code coverage and the test repos use Codecov to provide a coverage report w/ badges available at the top of the README. This doesn't affect I have shown with these test suites that ESM CLIs are viable from Node version In my next post I'll go through a detailed breakdown of my ESM @oclif/core fork regarding what needs to be changed. ~185 lines of additions / removals / changes brings ESM support to Oclif. The main bulk of the modifications are in @oclif/core, but of course there will be a few small changes necessary in some of the ancillary Oclif plugins. |
Now I would like to discuss the essential changes to My fork is I will also briefly reference my ESM Oclif middleware and active ESM CLI in development called fvttdev for an example use case.
The source files that require changes are:
New source file:
One module dependency and two developer dependencies in:
I'll start with the interface files:
Edit (06/01/21): The following links where applicable have been updated to point to the now merged / actual source code in @oclif/core itself which provides ESM support. Given ongoing work on Oclif v2 these links may not accurately resolve, but should get to the approximate area where changes have occurred. It makes sense to discuss the new file src/module-loader.ts next as it is utilized in This workaround is necessary as dynamic import / The two methods Now I would like to comment on the only non-essential feature added which is found in ModuleLoader. It wasn't hard to implement and adds functionality to loading hooks and the custom help class. That is the ability to specify in the Oclif config in package.json for a hook or custom help class to be an actual module or module export installed rather than just a local source file. This allows 3rd party modules to be loaded directly without having to re-export any resources from a local file. A use case for this is explicitly defining hook execution order for init / initializing. In the case of my Oclif middleware the init hook from the middleware needs to load first before the local CLI init hook, so order matters. Also I provide a default custom help class which can be directly referenced in the config. You can see this in use in To accomplish this and maintain present functionality of loading local files ModuleLoader.resolvePath first tries With an understanding of the ModuleLoader addition I'll comment on changes to The largest flow control change is to runHook in The old code only worked due to the synchronous nature of require() and underlying implementation in Node. If you run Oclif with The changes to The following is a discussion on the changes to loading custom help classes and how async loading affects the help subsystem. To load custom help classes that may be ESM So accordingly in I propose that the help subsystem be made asynchronous as custom help implementations may need to load the command class via the command config Besides the use case mentioned above I have a practical real world use case that requires async aspects to function. In my Oclif middleware I have a custom Command base class DynamicCommand which allows flags to be added dynamically to commands by other Oclif plugins installed / initialized. In my real world CLI The test files that require changes are:
These changes for The above discusses the essential changes to @oclif/core to support ESM. Further work to be done revolves around a discussion about the help subsystem going asynchronous. 185 lines of additions / changes / removal brings ESM support to Oclif v2 while maintaining all existing functionality. In the next post I'll discuss other small modifications which will need to occur in other Oclif plugins. |
In this post I will discuss some of the ramifications I'm aware of for the ESM changes in regard to the other mainline Oclif plugins. The main concern is the asynchronous loading aspects and the async help subsystem including the
Most of these changes are relatively minor. The readme command support in As mentioned in the first post I'd be glad to help assist in updating any of the main plugins that require changes due to the ESM loading addition to Perhaps point to any of the v2 repo / branches for the plugins when they are available for beta consumption and I'll do a full analysis. |
In this final post I will discuss a few quality of life aspects for ESM and Oclif in general.
These minor quality of life changes regard programmatic control of invoking Oclif from an ESM context. This is the bootstrap code from #!/usr/bin/env node
import url from 'url';
import flush from '@oclif/core/flush.js';
import { run, Errors } from '@oclif/core';
run(void 0, url.fileURLToPath(import.meta.url))
.then(flush)
.catch(Errors.handle); which can be condensed to: #!/usr/bin/env node
import { flush, run, Errors } from '@oclif/core';
run(void 0, import.meta.url)
.then(flush)
.catch(Errors.handle); In particular it is reasonable to support file URLs as string & URL so that The other suggestion of adding flush to the main export just streamlines import of all resources commonly necessary to invoke Oclif programmatically. Well that wraps everything up for now.. I remain hopeful to interface with the Oclif team before the v2 launch. IMHO launching without ESM support will bring a significant pain point to Oclif midstream during the v2 lifecycle; probably sooner than later as the move to ESM will be rapid for certain parts of the Node ecosystem. |
Wow @typhonrt - first, let me thank you for taking the time to contribute and for being so thorough. Now that you wrapped up the explanation, let me finish digesting this. I'm about halfway and still need to test some stuff out. I just wanted to engage to let you know we are looking at this. I agree that ESM is valuable and if we are going to add it, now is the time. |
We will be doing a major version bump of all oclif repositories when they transition over to oclif/core, so this type of breaking change is ok. With that said, we will probably need to document a migration guide from oclif/... to oclif/core. There will be an announcement issue we will be posting soon. I'll update this when we have that.
Both of those changes seem fine. I didn't see them in your fork though (PR nodejs/modules#144). Do you plan on adding those? Edit: Removed a previous suggestion as it creates syntax errors. |
@amphro Excellent to hear that this effort is going to get into Oclif v2! Thanks for indicating general approval w/ the PR as that was quite a surprise. I'm currently finishing up major refactoring / final polish to my Oclif v2 middleware efforts and should have that complete early next week and can get started on addressing the concerns in the PR and assist on updating other Oclif plugins as they become available for review along with documentation efforts. The initial fork / PR contains just the core changes necessary to support ESM. I figured it would be best to submit changes in a sequence of a few steps / additional PRs, so I avoided adding the quality of life changes and potential changes for the help subsystem going fully asynchronous. I have already prototyped the help subsystem changes in my middleware. I will get to writing responses to the comments you raised in the PR by early next week and I should be freed up at that time to continue the process. Let me know if the basic strategy of breaking things down into 2 PRs makes sense. |
@amphro Just wanted to drop a note that I'm still around. I actually will be finished with the coding queue I've been working through this week for sure. Looking forward to getting to Oclif v2 ESM PR and such soon. |
@amphro ... Alright.. I'm ready to get the process rolling and generally available on a more steady basis through the Oclif v2 launch. I have responded to your initial review in the current PR. As mentioned I think the best way to proceed is for me to make a new feature branch that includes your suggestions and also includes the quality of life changes I mentioned above along with |
Sounds good @typhonrt - I closed out the PR I created with some additional comments. If possible, try to have several smaller PRs that are focused: ESM, help, quality of life. I think it will make the PR reviews easier. I understand that may be more challenging as you have already made a bunch of changes on the main branch of your fork. I don't want to waste your time so if it will take too much, then one PR is fine. |
No worries, sounds like a plan.. That was probably my 4th attempt at adding ESM between v1 and v2 so 5th attempt is the charm and should be done soon; I have a reasonable understanding of the core. :D |
@amphro The first PR (#160) which just integrates ESM loading for commands and hooks is available. The second PR to integrate ESM loading for help classes depends on it (ModuleLoader), so I'll start on that after the first PR is merged. I'll hold off on the third QOL PR as well and just do them in order. |
@amphro Indeed there is a 2nd PR (#163) which significantly strengthens ModuleLoader and fixes an oversight from the initial PR (#160) and adds the ability to load all ESM plugins, mixed ESM w/ CJS, mixed CJS w/ ESM, and continuing support for TS loading. The ModuleLoader unit tests are improved, but more importantly there are now full Oclif config / runtime tests which will catch issues for ESM support during runtime. Had the runtime tests been added previously I wouldn't be making a second PR for core ESM support. These new runtime tests also show the versatility of ModuleLoader and mixed ESM / CJS plugin support. |
@amphro Thanks for the nodejs/modules#163 merge. I now have the asynchronous help subsystem PR nodejs/modules#165 available for review. Once this passes muster and is merged I can complete the final quality of life PR for ESM CLIs regarding invoking Oclif itself. |
nodejs/modules#165 looked good and I had no feedback, so it is merged. I wanted to make some of the quality of life improvements too but I have been holding off until this issue is closed. Let me know when your file PR is up for review. Once this is issue is closed, we will also be making some larger updates to the default help class which we think is much better UX for CLI help. |
@amphro Alrighty.. The ESM quality of life PR nodejs/modules#166 has been submitted for review. This rounds out all of the changes required to provide ace support for ESM via Oclif v2. Exciting days ahead. I'll be available to assist in any updates required to other Oclif plugins as they are made publicly available if necessary. I'd be glad to consult as well on documentation suggestions. I'll be updating all of my demo repos shortly once the latest PR is merged. Thanks for the opportunity to add ESM support to Oclif. I believe this will definitely give the edge for v2 when it comes to choice of CLI tooling and future proof it for the coming ESM wave on Node. |
Merged. I'm going to close this out. File any additional issues for anything else you find. |
Deploying an ESM CLI (06/01/21)Please review the following complete test cli repos which feature how to set up ESM Oclif v2 CLIs. Code coverage is gathered and the tests show how to use
Deployment of an ESM CLI on NPM for Oclif v2 does have a problematic area to overcome presently. While there is newly added ESM support to The contents of the lib directory from the above repository needs to be copied into Note: This workaround only works with When Oclif v2 fully launches no manual workarounds will be necessary. For a non-trivial example of an ESM Oclif v2 CLI that is published on NPM you can review my ongoing project fvttdev. |
@typhonrt LETS GOOOO, thank you so much for your hard work! |
Hey @typhonrt, No I want to combine with subcommands introduced here: Therefor i cloned the hello-world example and transformed it into ES-Module Unfortunately I missed something... Expected: (hello-world example from linked issuecomment) Error: (hello-world from phi bar/hello-world) The main difference is that "hello" command is now inside hello folder in index file. Perhaps you know what is going wrong here, would be great if you can give me a hint what to change, or where to investigate. |
Hi @phibar. Alas, I have been working on other projects other than my CLI based project, so haven't been keeping up with the current state of oclif v2. It's also a tough spot as I spent ~180 hours on researching & implementing the initial ESM support for Do update the repo link above and I can take a quick look to see if I see anything. |
Edit (06/01/21): Thanks to the Oclif team for letting me contribute ESM support to Oclif v2! I have updated all links in this issues comments to be as accurate as possible. Please see this comment on a current workaround to publish an ESM CLI while the rest of the Oclif v2 infrastructure is updated.
Hi Philipe & Thomas (@RasPhilCo / @amphro) et al,
Great work on Oclif v2 Philipe and all involved. I really like the refactor to a core module compared to v1. Removing the circular dependencies (IE between v1 plugin-help and plugin-command) helps a bunch. I would like to discuss adding ES Module (ESM) support to Oclif v2 before initial launch in ~June. I initially prototyped the changes in v1 and saw the core announcement and have fully worked out the essential changes to @oclif/core for v2. I have a fork of @oclif/core with these changes and have fully implemented non-trivial Oclif middleware and resulting CLI w/ ESM to test everything out that is published on NPM using my @oclif/core fork. I also have three test repositories that have stripped down CLI tests with variations that respectively cover a wide swath of the 12.0.0 to 15.x Node ecosystem by Github Actions. I'll comment on this below in another post.
This kind of fundamental change is best in a new version of Oclif and is timely due to the minimum Node support of v12 for Oclif v2 which is the first Node LTS version that has wider support for ES Modules. Supporting ESM now future proofs Oclif as the larger Node ecosystem transitions to ESM in the coming years. There is a fair amount of movement in this direction already in April / May as Node 12 becomes the minimum LTS release. These changes do not affect current support of CommonJS / Typescript CLIs. In fact there could be some benefit for end user CLI development in Typescript as one could target ESM instead of CJS and if launching a CLI w/ Node 14 as a minimum version for support can target ES2020 for Typescript or simply write the CLI in ESM / ES2020. The main core Oclif v2 code should still of course target CJS for now, but this frees up end users to choose their target of choice between CJS / ESM / Typescript.
I will go over in more detail in a follow up post to this issue regarding what the changes cover and note the other areas of the larger codebase / other modules that require changes. I have already gone through several iterations of improving the essential changes and have arrived at what I think is a clean addition which I have tested across all the OS platforms w/ Node
12.0.0
-15.x
. Beyond just submitting the pull requests / changes I am fully committed to helping write new tests / verifying Node 12.0.0 compatibility and assisting with writing documentation changes for v2. IE not just provide a code dump and add extra burden to any internal team members before the v2 launch. I believe there is enough time to get these changes in responsibly before the v2 ~June launch.Instead of just making a pull request to @oclif/core I'd like to discuss willingness to add ESM support through my involvement first. In follow up posts to this issue I'll discuss the essential changes to @oclif/core and other modules (plugin-help / dev-cli / oclif / test). I'm 100% open to modifying anything necessary before / while making any pull requests. I look forward to working with the Oclif team to support ESM and make Oclif v2 a very solid solution for advanced CLI development on Node now and into the future.
The text was updated successfully, but these errors were encountered: