Strengthened ModuleLoader & unit tests; now supports mixed ESM / CJS plugins #163
Conversation
|
I did decide to remove the unnecessary This makes it really obvious what has changed in this PR. |
test/config/mixed-cjs-esm.test.ts
Outdated
| withConfig | ||
| .stdout() | ||
| .it('runs init hook', async ctx => { | ||
| // to-do: fix union types |
There was a problem hiding this comment.
It isn't clear to me what this todo needs to fix
There was a problem hiding this comment.
These "to-do" statement are lifted directly from test/config/typescript.test.ts which the new ESM and mixed ESM / CJS tests used as the example runtime test except for changing around the type of source being tested.
However there does seem to be a reason for the comment being there; the line:
await (ctx.config.runHook as any) will cause TSC to post an error without the as any.
If you do track down the method signature in /src/interfaces there appears to be a union type.
Looks like @RasPhilCo can comment on it from looking at git history.
test/config/mixed-esm-cjs.test.ts
Outdated
| withConfig | ||
| .stdout() | ||
| .it('runs init hook', async ctx => { | ||
| // to-do: fix union types |
| configESM.pjson.type = 'module' | ||
|
|
||
| const result = await ModuleLoader.loadWithData(module.isESM ? configESM : config, module.path) | ||
| // const result = await ModuleLoader.loadWithData(module.isESM ? configESM : config, module.path) |
There was a problem hiding this comment.
Nope.. Left over from being able to remove creating a special config for ESM testing. The ModuleLoader update / fix is a bit more flexible now
|
@amphro Thanks for checking in.. I was going to give it 14 days before bumping this myself. I have made two more commits removing the "to-do" statements and left over commented out old implementation from the newly added runtime tests. Things are ready to merge if you are happy with the PR. Don't go too far though as I'll get the other two PRs in right away once this is merged. |
|
I'll keep an eye out for the additional PRs |
@amphro I do have a second PR which is pertinent. I made one mistake in the separate unit tests insofar that at the very tail end I relied on just the separate ModuleLoader unit test to pass outside of a full runtime use case and this was the one time I didn't test final changes against my ESM middleware / CLI.
What went wrong: In the
ModuleLoader.resolvePathmethod when I created the the bad path testsrequire.resolveused a 2nd time would throw an exception. I didn't add in tests for good paths w/ no extensions which is how Oclif works.require.resolvewill resolve the extension (.js). I switched topath.resolveand while the unit tests passed the aspect of not resolving the extension failed when running w/ my external ESM middleware & ESM CLI after #160 was merged.So the initial unit test while everything passed I made a small mistake in modifying the ModuleLoader code to satisfy the tests and didn't do a full runtime test.
This PR significantly hardens ModuleLoader restoring correct behavior. In addition I didn't want to go back to using
require.resolveand made a custom extension resolution algorithm that checks for (.js,.mjs,.cjs). There is a major benefit to this as I worked out a way to support mixed CJS / ESM loading of plugins. Oclif can now support fully ESM plugins w/"type": "module"in the package.json and all.jsfiles being ESM. There is also support now for loading ESM via.mjsfile extensions from a CommonJS plugin and vice versa with an ESM plugin w/"type": "module"one can load CJS via the.cjsextension.I strengthened the unit tests for ModuleLoader, but more importantly I added complete runtime tests for ESM, mixed ESM w/ CJS (
.cjs), and mixed CJS w/ ESM (.mjs) which you can find in./test/config/fixtures/->esm,mixed-cjs-esm, andmixed-esm-cjs.Note (edit update below): For the new runtime tests I initially I copied the style of
./test/config/fixtures/typescriptwhich includes an unnecessaryyarn.lockfile. I removed them from the ESM / CJS runtime tests in a commit on Friday.The files changed / count increase is from the new runtime tests.
And yeah I tested this PR w/ my external ESM Oclif middleware / CLI as well, so it is definitely fixed.
Not ideal per se w/ two PRs for the core ESM addition, but this PR definitely gets things there 100% and has full unit tests and newly added runtime tests to back it up.