Skip to content

Conversation

@guybedford
Copy link
Contributor

@guybedfordguybedford commented Nov 2, 2020

This updates to [email protected], 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-botnodejs-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
ContributorAuthor

Requesting a fast track here again.

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

@MylesBorinsMylesBorins 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

@targostargos 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
ContributorAuthor

Landed in a5f99b8.

@guybedfordguybedford 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 <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35928 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@targostargos 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 <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35928 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
PR-URL: #35928 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Dec 10, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-trackPRs that do not need to wait for 48 hours to land.lib / srcIssues 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

@guybedford@nodejs-github-bot@MylesBorins@bmeck@targos