-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Conversation
Review requested:
|
We should fast track this to get it in the next 15 and into 14 / 12 asap |
Is #35857 (comment) related to this? Will these node stdlib module getters not be available as named exports in esm? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber Stamp LGTM
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>
Landed in b92d2e4. Thanks all for the quick review here. |
I've added |
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>
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>
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>
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>
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>
This upgrades to cjs-module-lexer@0.5.0 which filters CJS named exports detection to not detect getter properties like:
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), orvcbuild test
(Windows) passes