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

module: add test for cjs loader with esm flag #27443

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 28, 2019

When using the --experimental-modules flag with node, the CJS loader crashes sometime aas described in the related issue. I don' t have sufficient background to understand the module loading, but I have managed to isolate the bug and right a test, hoping that would encourages someone with a better background to tackle the issue.

I have observed the bug to be triggered by stealthy-require module, but only when the --experimental-modules flag is specified, even if no .mjs are imported. This is going to be a problem when we remove the esm flag as the module has more than 4M weekly download on npm.

Refs: #25482

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 28, 2019
@Trott
Copy link
Member

Trott commented May 1, 2019

If the test is failing but should be passing, we could place it in the test/known_issues directory.

Did you write the test? If so, any chance you can remove the ISC license boilerplate? We normally expect code contributions to be distributed under the MIT license of Node.js.

@aduh95
Copy link
Contributor Author

aduh95 commented May 1, 2019

Most of the code in the test comes from the stealthy-require module, which is under the ISC license:
https://github.com/analog-nico/stealthy-require/blob/master/lib/index.js

I don't know if it's fine to release it under the MIT license, maybe we can get the feedback of the author.. @analog-nico, what do you think?

@analog-nico
Copy link

I am totally fine with removing the license note. After all it helps the node ecosystem. :)

Please put me in CC if you analyze this issue as I may need to update strealthy-require. I’ll also take a look at the issue soon.

@aduh95 aduh95 marked this pull request as ready for review May 2, 2019 01:59
@Trott Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 2, 2019
@Trott
Copy link
Member

Trott commented May 2, 2019

A small number of very minor lint failures that will need to be corrected.

/home/travis/build/nodejs/node/test/known_issues/test-esm-loader-cache-clearing.js
   2:1   error  Strings must use singlequote                         quotes
   6:9   error  Strings must use singlequote                         quotes
   7:28  error  Expected parentheses around arrow function argument  arrow-parens
  10:9   error  Strings must use singlequote                         quotes

You can run the linter locally with make lint-js (or, I think, vcbuild lint-js on Windows).

This test shows the regression introduced in v11.4.0: clearing out the
require.cache crashes node when using the `--experimental-modules` flag.
Refs: nodejs#25482
@aduh95 aduh95 mentioned this pull request May 3, 2019
2 tasks
@guybedford
Copy link
Contributor

Thanks so much for isolating this bug. I've merged this test into the fix branch at #25491.

@Trott
Copy link
Member

Trott commented May 13, 2019

Thanks so much for isolating this bug. I've merged this test into the fix branch at #25491.

@guybedford Any reason not to cherry-pick a3b07f6 into that branch to preserve authorship for @aduh95? (Not sure if they care of not, but might as well if there's no downside.)

@ZYSzys
Copy link
Member

ZYSzys commented May 14, 2019

Since It's cherry-picked into #25491 and landed in 523a9fb...ac3b98c, I'm going to close this.

@aduh95 Thanks your effort.

@ZYSzys ZYSzys closed this May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants