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@1.0.0 #35928

Closed
wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Nov 2, 2020

This updates to cjs-module-lexer@1.0.0, with some final getter refinements to fix #35859.

To explain the getter handling story here:

  1. The initial fix was to disable getters entirely to avoid destructive behaviours on import
  2. This broke Babel strict reexports, so instead lexing just a safe subset of getters that return value properties, or getters for identifiers and member expressions was supported.
  3. It turns out the original issue in (1) was still not resolved because the code of pg has an unsafe getter function, which internally does an Object.defineProperty once called to set a value property, which was then being detected as one of these safe properties.

Thus this update includes the fix at nodejs/cjs-module-lexer#29 which adds unsafe getter tracking to specifically mark unsafe getters and never emit them even if they later have safe getter forms.

I've verified this fixes the test case from #35859.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 2, 2020
@guybedford
Copy link
Contributor Author

Requesting a fast track here again.

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 2, 2020
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

RSLGTM

@MylesBorins
Copy link
Contributor

Can we please fast track?

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM with an update of the link in the docs

@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

Landed in a5f99b8.

@guybedford guybedford closed this Nov 2, 2020
@guybedford guybedford deleted the cjs-module-lexer branch November 2, 2020 18:24
guybedford added a commit that referenced this pull request Nov 2, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Nov 16, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
PR-URL: #35928
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.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
fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM importing a CommonJS calls getters
5 participants