deps: replace part of acorn with amaro - #66051
avivkeller wants to merge 1 commit into
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66051 +/- ##
=======================================
Coverage 90.23% 90.23%
=======================================
Files 789 789
Lines 270613 270527 -86
Branches 51801 51783 -18
=======================================
- Hits 244186 244120 -66
+ Misses 16885 16877 -8
+ Partials 9542 9530 -12
🚀 New features to boost your workflow:
|
|
Can you include benchmarks result from this PR? |
|
Why are there changes in |
| function resolveRelativeInDeps(request, parentId) { | ||
| const { posix } = requireBuiltin('path'); | ||
| const id = posix.join(posix.dirname(parentId), request); | ||
| // Builtin ids do not carry the file extension. | ||
| if (StringPrototypeEndsWith(id, '.js')) { | ||
| return StringPrototypeSlice(id, 0, -3); | ||
| } | ||
| if (StringPrototypeEndsWith(id, '.mjs')) { | ||
| return StringPrototypeSlice(id, 0, -4); | ||
| } | ||
| return id; | ||
| } |
There was a problem hiding this comment.
Shouldn't we vendor a bundle instead? Doing dynamic resolution is probably expansive, I don't think it's worth it
There was a problem hiding this comment.
Vendoring a bundle (of Amaro) makes it larger since one of our requirements was having a separate entrypoint for Node.js and non-Node.js consumers
There was a problem hiding this comment.
What about patching the files to rewrite the require calls? BTW we can forgo the .mjs one as we do not support ESM for builtins modules.
4 participants
We can't 100% remove Acorn just yet, there a few kinks to be worked around, but we can replace some of it, which this PR does.
When 100% done, this should improve the speed of assertions, highlighting, and reduce the size of the Node.js bundle.
cc @marco-ippolito