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

deps: upgrade to cjs-module-lexer@0.5.0 #35871

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

This upgrades to cjs-module-lexer@0.5.0 which filters CJS named exports detection to not detect getter properties like:

Object.defineProperty(exports, 'prop', {
  get () {
    throw new Error('error');
  }
});

The filtering is entirely handled by the lexer no longer emitting this case, while making an exception for __esModule.

Fixes #35859.

As a result, these cases which previously would be detected as named exports no longer come up as named exports, but it is a relatively minor edge case we expect.

Since the cases that this affects are relatively minor, and since this feature is still experimental we do not need to treat this as formally breaking and should instead aim to get this out asap to maximise compatibility.

Requesting to fast track this PR, please give a thumbs up or thumbs down here to help push it through.

@nodejs/modules-active-members

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@MylesBorins
Copy link
Member

We should fast track this to get it in the next 15 and into 14 / 12 asap

@MylesBorins MylesBorins added esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. labels Oct 29, 2020
@panva
Copy link
Member

panva commented Oct 29, 2020

Is #35857 (comment) related to this? Will these node stdlib module getters not be available as named exports in esm?

@guybedford
Copy link
Contributor Author

@panva conceptually it's the same thing, but it's different code running. The ESM sync with core libs happens in https://github.com/nodejs/node/blob/master/lib/internal/bootstrap/loaders.js#L237. In your PR I'd suggest either making those properties non enumerable or you can add some extra code in there that skips properties via a marker / whether they use a getter / or even just disables warnings. There's a few ways to go but I'm sure you'll be able to solve it there.

@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Rubber Stamp LGTM

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Oct 29, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@guybedford
Copy link
Contributor Author

Landed in b92d2e4. Thanks all for the quick review here.

@aduh95
Copy link
Contributor

aduh95 commented Oct 31, 2020

I've added dont-land labels in case this is related to #35893, feel free to remove them if it's unrelated or if the issue is a wont-fix.

targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Nov 16, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
PR-URL: #35871
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
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. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM importing a CommonJS calls getters
9 participants