-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
esm: implement import.meta.main #57804
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
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
135a7a1
to
f31d130
Compare
@ljharb I would appreciate your review on this r.e. performance - I am particularly concerned about my changes in lib/internal/modules/esm/translators.js, since I'm not familiar with implications of assignment to a JS object associated with a native object. |
5452731
to
b677069
Compare
6bdcad8
to
6613f1d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57804 +/- ##
==========================================
- Coverage 90.21% 90.20% -0.02%
==========================================
Files 630 630
Lines 186391 186421 +30
Branches 36608 36614 +6
==========================================
+ Hits 168161 168165 +4
- Misses 11052 11069 +17
- Partials 7178 7187 +9
🚀 New features to boost your workflow:
|
c4619a4
to
eb4e8a7
Compare
@ljharb I've moved the assignment into I think I can see where the cache is populated, but not where it is hit in What am I missing? |
I think CI should pass now. |
I checked locally, it seems the test no longer fails with unusual character names using a relative import path. |
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.
lgtm
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
There's a test for |
There is:
But it would still be nice to cover more cases, e.g.:
|
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
…enders to paragraphs
fa58a71
to
3f8416a
Compare
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.
lgtm
This should land after #58079 |
Implements #57226
This PR supersedes #32223
Fixes: #57226