-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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: update cjs-module-lexer to 2.0.0 #56855
deps: update cjs-module-lexer to 2.0.0 #56855
Conversation
Review requested:
|
This should be landed in preference to the autogenerated PR as in addition to the update it:
It was generated by adjusting the files as needed, and then running the update, followed by updating the test that I discovered needed adjustment. |
@marco-ippolito FYI |
Signed-off-by: Michael Dawson <mdawson@devrus.com>
24da6e7
to
a0e6994
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.
RSLGTM
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, off topic, since cjs-module-lexer is written in C we could build it directly without requiring a wasm?
Documentation needs to be updated. node/doc/contributing/maintaining/maintaining-cjs-module-lexer.md Lines 47 to 49 in 69502d8
Line 1112 in 69502d8
Line 1147 in 69502d8
Ideally the updater should take care of this. |
I think that would need some actual binding code to be written (or FFI support ;)) because right now it just manually exports selected functions through wasm and let the JS side drive them. It might be a fair bit faster if it can just work with buffers and we write some V8 fast APIs to work with the buffer, and refactor the ESM loader to simply pass UTF8 data in buffers around. |
Signed-off-by: Michael Dawson <midawson@redhat.com>
@richardlau updated the doc, can look at improving updater in a follow on PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56855 +/- ##
==========================================
- Coverage 89.20% 89.18% -0.02%
==========================================
Files 663 665 +2
Lines 192012 192529 +517
Branches 36929 37036 +107
==========================================
+ Hits 171286 171709 +423
- Misses 13582 13637 +55
- Partials 7144 7183 +39 |
Landed in dd92abc |
Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #56855 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
No description provided.