-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
lib: skip source maps in node_modules #56639
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
fb1a57e
to
cfc1fd9
Compare
* `nodeModules` {boolean} If enabling the support for files in `node_modules`. | ||
* `generatedCode` {boolean} If enabling the support for generated code from `eval` or `new Function`. |
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.
It's missing doc for default values I think
validateBoolean(enabled, 'enabled'); | ||
validateObject(options, 'options'); | ||
|
||
const { nodeModules = false, generatedCode = false } = options; |
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.
In the past, we always enabled the source map for nodeModules right? If so, having the default values to false
won't be a breaking change? Maybe having this as true
will be better if the my first assumption is right.
nodeModules: false, | ||
generatedCode: false, |
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.
Same for this one, this is probably breaking change if we keep false
.
Skipping source maps in `node_modules` by default improves the general performance. Add `module.setSourceMapsSupport(enabled, options)` to enable source maps in `node_modules` if it is needed. This moves all source maps related API to `node:module` and this a step to promote the source maps API to stable. Files in `node_modules` are not authored by the user directly and the original sources are less relevant to the user.
cfc1fd9
to
94667aa
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.
Considering many enable source maps via --enable-source-maps
I think it also warrants something like --disable-source-maps-in-node-modules
or some other flag with a better name? Otherwise end users have to preload a script that invokes the API which is a bit of a hassle. We can set it to false by default for existing releases and in the next major release, flip it to true.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56639 +/- ##
=======================================
Coverage 89.20% 89.21%
=======================================
Files 662 662
Lines 191890 191896 +6
Branches 36928 36938 +10
=======================================
+ Hits 171176 171194 +18
+ Misses 13554 13542 -12
Partials 7160 7160
|
Skipping source maps in
node_modules
by default improves the generalperformance. Add
module.setSourceMapsSupport(enabled, options)
toenable source maps in
node_modules
if it is needed. This movesall source maps related API to
node:module
and this a step topromote the source maps API to stable.
Files in
node_modules
are not authored by the user directly and theoriginal sources are less relevant to the user.
Micro-benchmark result: