Switch from CJS to ESM #859
Conversation
* switch project to type=module * adopt import/export syntax throughout codebase * update meow (ESM support since v10) * remove global-tunnel-ng branch since we're > Node 10 * replace proxyquire (no ESM) with testdouble * replace nyc (no ESM) with c8 * shim __dirname * remove unused --no-update-notifier in CLI * [WIP] disable failing tabtab test
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
| // `global-tunnel-ng` works only with Node.js v10 and below. | ||
| require('global-tunnel-ng').initialize(); | ||
| } | ||
| bootstrap(); |
There was a problem hiding this comment.
We can eliminate this branching because we're > Node 10.
Simply import and call bootstrap.
| @@ -33,7 +28,7 @@ const tabtab = new Tabtab.Commands.default({ | |||
| }); | |||
|
|
|||
| const cli = gens.map(gen => { | |||
| const minicli = meow({autoHelp: false, autoVersion: true, pkg, argv: gen}); | |||
| const minicli = meow({autoHelp: false, autoVersion: true, pkg, argv: gen, importMeta: import.meta}); | |||
There was a problem hiding this comment.
Small tweak to support the latest meow version.
| router.registerRoute('install', routes.install); | ||
| router.registerRoute('exit', routes.exit); | ||
| router.registerRoute('clearConfig', routes.clearConfig); | ||
| router.registerRoute('home', routes.home); |
There was a problem hiding this comment.
I collected these into a routes object that can be imported at once.
| @@ -53,7 +52,7 @@ module.exports = async app => { | |||
| } | |||
|
|
|||
| return fullname().then(name => { | |||
| const allo = (name && isString(name)) ? `'Allo ${name.split(' ')[0]}! ` : '\'Allo! '; | |||
| const allo = (name && _.isString(name)) ? `'Allo ${name.split(' ')[0]}! ` : '\'Allo! '; | |||
There was a problem hiding this comment.
I will occasionally inline lodash if it's a one-off and the specific method was imported directly.
| */ | ||
| export const getDirname = fileUrl => { | ||
| return path.dirname(fileURLToPath(fileUrl)); | ||
| }; |
There was a problem hiding this comment.
__dirname shim.
I did not write a corresponding test for this because __dirname is used all over the place anyway and failures here would show up as failures there. Also, it's tiny and the test would be 99% mock setup.
| "test": "nyc mocha --timeout=30000", | ||
| "coverage": "nyc report --reporter=text-lcov | coveralls" | ||
| "test": "c8 mocha --timeout=30000", | ||
| "coverage": "c8 report --reporter=text-lcov | coveralls" |
There was a problem hiding this comment.
nycdoesn't provide coverage for ESM projects (Nyc on ESM Node.js 13 (no babel) istanbuljs/nyc#1287)esm-loader-hookis promising but experimental (https://github.com/istanbuljs/esm-loader-hook)- c8 is a fully-compatible replacement that worked immediately
| "registry-url": "^5.1.0", | ||
| "sinon": "^19.0.2", | ||
| "testdouble": "^3.20.2", |
There was a problem hiding this comment.
Also considered sinon and rewiremock as alternatives here but this was the most seamless.
If this were my project, I'd switch to jest and use their module mocking.
| }); | ||
| }); | ||
|
|
||
| it('should return the version', cb => { | ||
| const cp = execFile('node', [ | ||
| path.resolve(__dirname, '..', pkg.bin.yo), | ||
| '--version', | ||
| '--no-update-notifier' |
There was a problem hiding this comment.
I couldn't find any evidence that this is used anywhere.
| @@ -31,7 +34,7 @@ describe('Completion', () => { | |||
| this.env = createEnv(); | |||
| }); | |||
|
|
|||
| describe('Test completion STDOUT output', () => { | |||
| describe.skip('Test completion STDOUT output', () => { | |||
There was a problem hiding this comment.
WIP - see comment about tabtab relying on module.parent
WIP / HOLD MERGE
module.parentand doesn't work with ESMThis PR migrates Yo to ECMAScript modules (ESM).
Most of the lines changed are related to the import/export syntax itself.
However, a few libraries and techniques had to be updated in order to work around older and non-ESM issue. I've listed those changes below.
Purpose of this pull request?
Switch from CJS to ESM in order to solve issue 787.
What changes did you make?
Is there anything you'd like reviewers to focus on?