Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions benchmark/es/error-stack.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

const common = require('../common.js');
const modPath = require.resolve('../fixtures/simple-error-stack.js');
const nodeModulePath = require.resolve('../fixtures/node_modules/error-stack/simple-error-stack.js');

const bench = common.createBenchmark(main, {
method: ['without-sourcemap', 'sourcemap'],
method: ['without-sourcemap', 'sourcemap', 'node-module-sourcemap', 'node-module'],
n: [1e5],
});

function runN(n) {
function runN(n, modPath) {
delete require.cache[modPath];
const mod = require(modPath);
bench.start();
Expand All @@ -22,11 +23,15 @@ function main({ n, method }) {
switch (method) {
case 'without-sourcemap':
process.setSourceMapsEnabled(false);
runN(n);
runN(n, modPath);
break;
case 'sourcemap':
process.setSourceMapsEnabled(true);
runN(n);
runN(n, modPath);
break;
case 'sourcemap-with-node-modules':
process.setSourceMapsEnabled(true);
runN(n, nodeModulePath);
break;
default:
throw new Error(`Unexpected method "${method}"`);
Expand Down
16 changes: 16 additions & 0 deletions benchmark/fixtures/node_modules/error-stack/simple-error-stack.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions benchmark/fixtures/node_modules/error-stack/simple-error-stack.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 38 additions & 0 deletions doc/api/module.md
Original file line number Diff line number Diff line change
Expand Up @@ -1587,6 +1587,43 @@ import { findSourceMap, SourceMap } from 'node:module';
const { findSourceMap, SourceMap } = require('node:module');
```
### `module.setSourceMapsSupport(enabled[, options])`
<!-- YAML
added: REPLACEME
-->
* `enabled` {boolean} Enable the source map support.
* `options` {Object} Optional
* `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`.
Comment on lines +1598 to +1599
Copy link
Member

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

This function enables or disables the [Source Map v3][Source Map] support for
stack traces.
It provides same features as launching Node.js process with commandline options
`--enable-source-maps`, with additional options to alter the support for files
in `node_modules` or generated codes.
Only source maps in JavaScript files that are loaded after source maps has been
enabled will be parsed and loaded. Preferably, use the commandline options
`--enable-source-maps` to avoid losing track of source maps of modules loaded
before this API call.
### `module.getSourceMapsSupport()`
<!-- YAML
added: REPLACEME
-->
* Returns: {Object}
* `enabled` {boolean} If the source maps support is enabled
* `nodeModules` {boolean} If the support is enabled for files in `node_modules`.
* `generatedCode` {boolean} If the support is enabled for generated code from `eval` or `new Function`.
This method returns whether the [Source Map v3][Source Map] support for stack
traces is enabled.
<!-- Anchors to make sure old links find a target -->
<a id="module_module_findsourcemap_path_error"></a>
Expand Down Expand Up @@ -1705,6 +1742,7 @@ returned object contains the following keys:
[Conditional exports]: packages.md#conditional-exports
[Customization hooks]: #customization-hooks
[ES Modules]: esm.md
[Source Map]: https://sourcemaps.info/spec.html
[Source map v3 format]: https://sourcemaps.info/spec.html#h.mofvlxcwqzej
[V8 JavaScript code coverage]: https://v8project.blogspot.com/2017/12/javascript-code-coverage.html
[V8 code cache]: https://v8.dev/blog/code-caching-for-devs
Expand Down
14 changes: 12 additions & 2 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -3995,9 +3995,13 @@ This feature is not available in [`Worker`][] threads.
added:
- v16.6.0
- v14.18.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/56639
description: the `process.setSourceMapsEnabled` has been deprecated.
-->
> Stability: 1 - Experimental
> Stability: 0 - Deprecated: Use [`module.setSourceMapsSupport()`][] instead.
* `val` {boolean}
Expand Down Expand Up @@ -4042,9 +4046,13 @@ Using this function is mutually exclusive with using the deprecated
added:
- v20.7.0
- v18.19.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/56639
description: the `process.sourceMapsEnabled` has been deprecated.
-->
> Stability: 1 - Experimental
> Stability: 0 - Deprecated: Use [`module.getSourceMapsSupport()`][] instead.
* {boolean}
Expand Down Expand Up @@ -4511,7 +4519,9 @@ cases:
[`console.error()`]: console.md#consoleerrordata-args
[`console.log()`]: console.md#consolelogdata-args
[`domain`]: domain.md
[`module.getSourceMapsSupport()`]: module.md#modulegetsourcemapssupport
[`module.isBuiltin(id)`]: module.md#moduleisbuiltinmodulename
[`module.setSourceMapsSupport()`]: module.md#modulesetsourcemapssupportenabled-options
[`net.Server`]: net.md#class-netserver
[`net.Socket`]: net.md#class-netsocket
[`os.constants.dlopen`]: os.md#dlopen-constants
Expand Down
10 changes: 6 additions & 4 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,8 @@ internalBinding('process_methods').setEmitWarningSync(emitWarningSync);

{
const {
getSourceMapsEnabled,
setSourceMapsEnabled,
getSourceMapsSupport,
setSourceMapsSupport,
maybeCacheGeneratedSourceMap,
} = require('internal/source_map/source_map_cache');
const {
Expand All @@ -381,10 +381,12 @@ internalBinding('process_methods').setEmitWarningSync(emitWarningSync);
enumerable: true,
configurable: true,
get() {
return getSourceMapsEnabled();
return getSourceMapsSupport().enabled;
},
});
process.setSourceMapsEnabled = setSourceMapsEnabled;
process.setSourceMapsEnabled = function setSourceMapsEnabled(val) {
setSourceMapsSupport(val);
};
// The C++ land calls back to maybeCacheGeneratedSourceMap()
// when code is generated by user with eval() or new Function()
// to cache the source maps from the evaluated code, if any.
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const {
} = internalBinding('util');
const { decorateErrorStack, kEmptyObject } = require('internal/util');
const {
getSourceMapsEnabled,
getSourceMapsSupport,
} = require('internal/source_map/source_map_cache');
const assert = require('internal/assert');
const resolvedPromise = PromiseResolve();
Expand Down Expand Up @@ -186,7 +186,7 @@ class ModuleJob extends ModuleJobBase {
// of missing named export. This is currently not possible because
// stack trace originates in module_job, not the file itself. A hidden
// symbol with filename could be set in node_errors.cc to facilitate this.
if (!getSourceMapsEnabled() &&
if (!getSourceMapsSupport().enabled &&
StringPrototypeIncludes(e.message,
' does not provide an export named')) {
const splitStack = StringPrototypeSplit(e.stack, '\n');
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -618,9 +618,9 @@ function initializeESMLoader(forceDefaultLoader) {

function initializeSourceMapsHandlers() {
const {
setSourceMapsEnabled,
setSourceMapsSupport,
} = require('internal/source_map/source_map_cache');
setSourceMapsEnabled(getOptionValue('--enable-source-maps'));
setSourceMapsSupport(getOptionValue('--enable-source-maps'));
}

function initializeFrozenIntrinsics() {
Expand Down
65 changes: 46 additions & 19 deletions lib/internal/source_map/source_map_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const {
ArrayPrototypePush,
JSONParse,
ObjectFreeze,
RegExpPrototypeExec,
SafeMap,
StringPrototypeCodePointAt,
Expand All @@ -15,15 +16,15 @@ let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
debug = fn;
});

const { validateBoolean } = require('internal/validators');
const { validateBoolean, validateObject } = require('internal/validators');
const {
setSourceMapsEnabled: setSourceMapsNative,
} = internalBinding('errors');
const {
defaultPrepareStackTrace,
setInternalPrepareStackTrace,
} = require('internal/errors');
const { getLazy } = require('internal/util');
const { getLazy, isUnderNodeModules, kEmptyObject } = require('internal/util');

const getModuleSourceMapCache = getLazy(() => {
const { SourceMapCacheMap } = require('internal/source_map/source_map_cache_map');
Expand All @@ -45,30 +46,48 @@ const { fileURLToPath, pathToFileURL, URL, URLParse } = require('internal/url');
let SourceMap;

// This is configured with --enable-source-maps during pre-execution.
let sourceMapsEnabled = false;
function getSourceMapsEnabled() {
return sourceMapsEnabled;
let sourceMapsSupport = ObjectFreeze({
__proto__: null,
enabled: false,
nodeModules: false,
generatedCode: false,
Comment on lines +52 to +53
Copy link
Member

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.

});
function getSourceMapsSupport() {
// Return a read-only object.
return sourceMapsSupport;
}

/**
* Enables or disables source maps programmatically.
* @param {boolean} val
* @param {boolean} enabled
* @param {object} options
* @param {boolean} [options.nodeModules]
* @param {boolean} [options.generatedCode]
*/
function setSourceMapsEnabled(val) {
validateBoolean(val, 'val');
function setSourceMapsSupport(enabled, options = kEmptyObject) {
validateBoolean(enabled, 'enabled');
validateObject(options, 'options');

const { nodeModules = false, generatedCode = false } = options;
Copy link
Member

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.

validateBoolean(nodeModules, 'options.nodeModules');
validateBoolean(generatedCode, 'options.generatedCode');

setSourceMapsNative(val);
if (val) {
setSourceMapsNative(enabled);
if (enabled) {
const {
prepareStackTraceWithSourceMaps,
} = require('internal/source_map/prepare_stack_trace');
setInternalPrepareStackTrace(prepareStackTraceWithSourceMaps);
} else if (sourceMapsEnabled !== undefined) {
// Reset prepare stack trace callback only when disabling source maps.
} else {
setInternalPrepareStackTrace(defaultPrepareStackTrace);
}

sourceMapsEnabled = val;
sourceMapsSupport = ObjectFreeze({
__proto__: null,
enabled,
nodeModules: nodeModules,
generatedCode: generatedCode,
});
}

/**
Expand Down Expand Up @@ -130,14 +149,18 @@ function extractSourceMapURLMagicComment(content) {
* @param {string | undefined} sourceMapURL - the source map url
*/
function maybeCacheSourceMap(filename, content, moduleInstance, isGeneratedSource, sourceURL, sourceMapURL) {
const sourceMapsEnabled = getSourceMapsEnabled();
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
const support = getSourceMapsSupport();
if (!(process.env.NODE_V8_COVERAGE || support.enabled)) return;
const { normalizeReferrerURL } = require('internal/modules/helpers');
filename = normalizeReferrerURL(filename);
if (filename === undefined) {
// This is most likely an invalid filename in sourceURL of [eval]-wrapper.
return;
}
if (!support.nodeModules && isUnderNodeModules(filename)) {
// Skip file under node_modules if not enabled.
return;
}

if (sourceMapURL === undefined) {
sourceMapURL = extractSourceMapURLMagicComment(content);
Expand Down Expand Up @@ -185,8 +208,8 @@ function maybeCacheSourceMap(filename, content, moduleInstance, isGeneratedSourc
* @param {string} content - the eval'd source code
*/
function maybeCacheGeneratedSourceMap(content) {
const sourceMapsEnabled = getSourceMapsEnabled();
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
const support = getSourceMapsSupport();
if (!(process.env.NODE_V8_COVERAGE || support.enabled || support.generated)) return;

const sourceURL = extractSourceURLMagicComment(content);
if (sourceURL === null) {
Expand Down Expand Up @@ -352,6 +375,10 @@ function findSourceMap(sourceURL) {
return undefined;
}

if (!getSourceMapsSupport().nodeModules && isUnderNodeModules(sourceURL)) {
return undefined;
}

SourceMap ??= require('internal/source_map/source_map').SourceMap;
try {
if (RegExpPrototypeExec(kLeadingProtocol, sourceURL) === null) {
Expand All @@ -377,8 +404,8 @@ function findSourceMap(sourceURL) {

module.exports = {
findSourceMap,
getSourceMapsEnabled,
setSourceMapsEnabled,
getSourceMapsSupport,
setSourceMapsSupport,
maybeCacheSourceMap,
maybeCacheGeneratedSourceMap,
sourceMapCacheToObject,
Expand Down
18 changes: 14 additions & 4 deletions lib/module.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
'use strict';

const { findSourceMap } = require('internal/source_map/source_map_cache');
const {
findSourceMap,
getSourceMapsSupport,
setSourceMapsSupport,
} = require('internal/source_map/source_map_cache');
const { Module } = require('internal/modules/cjs/loader');
const { register } = require('internal/modules/esm/loader');
const { SourceMap } = require('internal/source_map/source_map');
const {
SourceMap,
} = require('internal/source_map/source_map');
const {
constants,
enableCompileCache,
Expand All @@ -15,14 +21,18 @@ const {
} = require('internal/modules/package_json_reader');
const { stripTypeScriptTypes } = require('internal/modules/typescript');

Module.findSourceMap = findSourceMap;
Module.register = register;
Module.SourceMap = SourceMap;
Module.constants = constants;
Module.enableCompileCache = enableCompileCache;
Module.findPackageJSON = findPackageJSON;
Module.flushCompileCache = flushCompileCache;
Module.getCompileCacheDir = getCompileCacheDir;
Module.stripTypeScriptTypes = stripTypeScriptTypes;

// SourceMap APIs
Module.findSourceMap = findSourceMap;
Module.SourceMap = SourceMap;
Module.getSourceMapsSupport = getSourceMapsSupport;
Module.setSourceMapsSupport = setSourceMapsSupport;

module.exports = Module;
Loading
Loading