Skip to content
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

enable esm support in Node v10 #4299

Merged
merged 4 commits into from Jun 5, 2020
Merged

enable esm support in Node v10 #4299

merged 4 commits into from Jun 5, 2020

Conversation

giltayar
Copy link
Contributor

Description of the Change

Fixes #4297. It seems that Mocha works with Node v10. So I enabled it to work in Node v10, but added documentation to say that it isn't officially supported and use at your own risk.

@boneskull boneskull added semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal labels May 27, 2020
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, but we should probably add a test for this to be run on node 10. it could be something that checks the node version in a before hook, then runs invokeMocha with the --experimental-modules flag (this should run bin/mocha)

@boneskull
Copy link
Member

would prefer another opinion from @mochajs/core but yeah, I think it's worth testing. @giltayar if you don't have the cycles to add it, we can merge and I can add it after.

@giltayar
Copy link
Contributor Author

@boneskull I dunno. I don't think adding a test for v10 is worth the time and hassle. I don't see many people using Node.js v10 with modules. I certainly won't recommend it given that Node v10 ESM support is incompatible in certain ways with v>=12 Node support. And v10 is not the active LTS and is going to EOL mid next year.

@giltayar
Copy link
Contributor Author

But your call, @boneskull. If you think it's worth it, let's merge, and I will add the test.

@boneskull
Copy link
Member

my reasoning is that if we support node 10, people will be upset if we break it, and we won’t know if we break it unless there’s a test.

@giltayar
Copy link
Contributor Author

@boneskull if y'all think it's worth testing, then I'll add a test.

@giltayar
Copy link
Contributor Author

OK, the whole conversation took more than the change to also test it in Node v10. Sorry about that! ☺️

@coveralls
Copy link

coveralls commented May 28, 2020

Coverage Status

Coverage increased (+0.03%) to 93.596% when pulling 0ce4c75 on giltayar:master into 63eb80b on mochajs:master.

@boneskull
Copy link
Member

netlify is being ornery; failure is unrelated

@@ -39,6 +39,8 @@ describe('esm', function() {
});

it('should recognize esm files ending with .js due to package.json type flag', function(done) {
if (!utils.supportsEsModules(true)) return done();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use this.skip() as well, but it doesn't really matter.

@boneskull
Copy link
Member

Resolved conflicts

@juergba
Copy link
Member

juergba commented Jun 3, 2020

@giltayar your branch is broken, could you have a look, please?

@giltayar
Copy link
Contributor Author

giltayar commented Jun 3, 2020

Some new ESM tests failed in Node v10. Specifically, those that assume that type:module in the package.json works, which it doesn't in Node 10.

The function supportEsmModules in this PR was changed to enable asking whether the ESM support is full (v>=12) or partial (v10). The default was partial, unfortunately, so any new test that checked if it had to run got true on v10, and failed on tests that assumed "type:module" works.

I switched the supportEsmModules function to default to "full support", so that if you want to specifically check for Node v10 support you can, but by default your tests run only for Node that has full ESM support.

@giltayar
Copy link
Contributor Author

giltayar commented Jun 3, 2020

I believe the Netlify failure is unrelated to this PR.

@juergba
Copy link
Member

juergba commented Jun 3, 2020

I don't know, it's the second time that netlify fails. Something is weird about you and netlify.
There shouldn't be a package-lock.json in your last commit, I think.

@giltayar
Copy link
Contributor Author

giltayar commented Jun 3, 2020

The package-lock.json change was strange: my commit changed some URLs from https to http, and I have no idea why. I'm pushing a change that reverts that and we'll see, but frankly, I don't think that's it.

I also tried looking at the problem at Netlify and it said the page was not found or I have insufficient permissions.

@giltayar
Copy link
Contributor Author

giltayar commented Jun 3, 2020

@juergba reverting the package-lock.json didn't work, and I can't access the netlify deploy preview details. Could you have a look at it, or if possible, give me permissions there? (My Netlify user[ is gil@tayar.org)

@juergba
Copy link
Member

juergba commented Jun 3, 2020

@giltayar I can't see your netlify log (page not found) neither. I can see the deploy preview of all the others (maintainers, contributors), but not yours. You are special.
Ask @Munter for support, he may know.

@boneskull
Copy link
Member

(It's not showing because the branch is on his fork, not on mochajs/mocha)

@boneskull
Copy link
Member

It's failing due to a caching problem

@boneskull
Copy link
Member

I've rebased onto master; hopefully this will pass CI.

@giltayar: you opened this PR from your fork's master branch, which is problematic 😄

@boneskull
Copy link
Member

clearing the cache before force-rebuilding worked.

the problem is that we needed to add an .nvmrc to our repo to force Node 12 for netlify. but since previous deploys were running probably Node 14, the cache contained native modules compiled against a different ABI version, so ... the only way to fix it is to blast the cache.

fwiw, I don't like the .nvmrc; we can do this another way. see #4314

@boneskull
Copy link
Member

here's the relevant travis build which, for some reason, I'm not seeing in the list of checks.

@boneskull boneskull merged commit a2f2e08 into mochajs:master Jun 5, 2020
@boneskull boneskull added this to the next milestone Jun 5, 2020
@giltayar
Copy link
Contributor Author

giltayar commented Jun 6, 2020

@giltayar: you opened this PR from your fork's master branch, which is problematic 😄

Sorry. Learning about OSS as I go along. Won't happen again! ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please don't prevent ES modules from being used on older Node.js versions
4 participants