8000 module: fix conditions override in synchronous resolve hooks · nodejs/node@fe0195f · GitHub
[go: up one dir, main page]
Skip to content

Commit fe0195f

Browse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
module: fix conditions override in synchronous resolve hooks
1. Make sure that the conditions are converted into arrays when being passed into user hooks. 2. Pass the conditions from user hooks into the ESM resolution so that it takes effect. PR-URL: #59011 Fixes: #59003 Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Jacob Smith <jacob@frende.me>
1 parent 55a90ee commit fe0195f

File tree

10 files changed

+269
-33
lines changed
  • lib/internal/modules
    • < 8000 div style="width:100%;display:flex">
      cjs
  • esm
  • helpers.js
  • test

    • 10 files changed

    +269
    -33
    lines changed

    ‎lib/internal/modules/cjs/loader.js‎

    Lines changed: 22 additions & 6 deletions
    Original file line numberDiff line numberDiff line change
    @@ -51,6 +51,7 @@ const {
    5151
    ReflectSet,
    5252
    RegExpPrototypeExec,
    5353
    SafeMap,
    54+
    SafeSet,
    5455
    String,
    5556
    StringPrototypeCharAt,
    5657
    StringPrototypeCharCodeAt,
    @@ -154,6 +155,7 @@ const internalFsBinding = internalBinding('fs');
    154155
    const { safeGetenv } = internalBinding('credentials');
    155156
    const {
    156157
    getCjsConditions,
    158+
    getCjsConditionsArray,
    157159
    initializeCjsConditions,
    158160
    loadBuiltinModule,
    159161
    makeRequireFunction,
    @@ -653,7 +655,7 @@ const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)?$/;
    653655
    * Resolves the exports for a given module path and request.
    654656
    * @param {string} nmPath The path to the module.
    655657
    * @param {string} request The request for the module.
    656-
    * @param {unknown} conditions
    658+
    * @param {Set<string>} conditions The conditions to use for resolution.
    657659
    * @returns {undefined|string}
    658660
    */
    659661
    function resolveExports(nmPath, request, conditions) {
    @@ -1068,17 +1070,30 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
    10681070
    function defaultResolve(specifier, context) {
    10691071
    // TODO(joyeecheung): parent and isMain should be part of context, then we
    10701072
    // no longer need to use a different defaultResolve for every resolution.
    1073+
    // In the hooks, context.conditions is passed around as an array, but internally
    1074+
    // the resolution helpers expect a SafeSet. Do the conversion here.
    1075+
    let conditionSet;
    1076+
    const conditions = context.conditions;
    1077+
    if (conditions !== undefined && conditions !== getCjsConditionsArray()) {
    1078+
    if (!ArrayIsArray(conditions)) {
    1079+
    throw new ERR_INVALID_ARG_VALUE('context.conditions', conditions,
    1080+
    'expected an array');
    1081+
    }
    1082+
    conditionSet = new SafeSet(conditions);
    1083+
    } else {
    1084+
    conditionSet = getCjsConditions();
    1085+
    }
    10711086
    defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, {
    10721087
    __proto__: null,
    1073-
    conditions: context.conditions,
    1088+
    conditions: conditionSet,
    10741089
    });
    10751090

    10761091
    defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename);
    10771092
    return { __proto__: null, url: defaultResolvedURL };
    10781093
    }
    10791094

    10801095
    const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined,
    1081-
    getCjsConditions(), defaultResolve);
    1096+
    getCjsConditionsArray(), defaultResolve);
    10821097
    const { url } = resolveResult;
    10831098
    format = resolveResult.format;
    10841099

    @@ -1154,7 +1169,7 @@ function loadBuiltinWithHooks(id, url, format) {
    11541169
    url ??= `node:${id}`;
    11551170
    // TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
    11561171
    const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
    1157-
    getCjsConditions(), getDefaultLoad(url, id));
    1172+
    getCjsConditionsArray(), getDefaultLoad(url, id));
    11581173
    if (loadResult.format && loadResult.format !== 'builtin') {
    11591174
    return undefined; // Format has been overridden, return undefined for the caller to continue loading.
    11601175
    }
    @@ -1306,7 +1321,7 @@ Module._load = function(request, parent, isMain) {
    13061321
    * @param {ResolveFilenameOptions} options Options object
    13071322
    * @typedef {object} ResolveFilenameOptions
    13081323
    * @property {string[]} paths Paths to search for modules in
    1309-
    * @property {string[]} conditions Conditions used for resolution.
    1324+
    * @property {Set<string>?} conditions The conditions to use for resolution.
    13101325
    * @returns {void|string}
    13111326
    */
    13121327
    Module._resolveFilename = function(request, parent, isMain, options) {
    @@ -1755,7 +1770,8 @@ function loadSource(mod, filename, formatFromNode) {
    17551770
    mod[kURL] = convertCJSFilenameToURL(filename);
    17561771
    }
    17571772

    1758-
    const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, getCjsConditions(),
    1773+
    const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined,
    1774+
    getCjsConditionsArray(),
    17591775
    getDefaultLoad(mod[kURL], filename));
    17601776

    17611777
    // Reset the module properties with load hook results.

    ‎lib/internal/modules/esm/loader.js‎

    Lines changed: 17 additions & 24 deletions
    Original file line numberDiff line numberDiff line change
    @@ -709,24 +709,30 @@ class ModuleLoader {
    709709
    if (this.#customizations) { // Only has module.register hooks.
    710710
    return this.#customizations.resolve(specifier, parentURL, importAttributes);
    711711
    }
    712-
    return this.#cachedDefaultResolve(specifier, parentURL, importAttributes);
    712+
    return this.#cachedDefaultResolve(specifier, {
    713+
    __proto__: null,
    714+
    conditions: this.#defaultConditions,
    715+
    parentURL,
    716+
    importAttributes,
    717+
    });
    713718
    }
    714719

    715720
    /**
    716721
    * Either return a cached resolution, or perform the default resolution which is synchronous, and
    717722
    * cache the result.
    718723
    * @param {string} specifier See {@link resolve}.
    719-
    * @param {string} [parentURL] See {@link resolve}.
    720-
    * @param {ImportAttributes} importAttributes See {@link resolve}.
    724+
    * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
    721725
    * @returns {{ format: string, url: string }}
    722726
    */
    723-
    #cachedDefaultResolve(specifier, parentURL, importAttributes) {
    727+
    #cachedDefaultResolve(specifier, context) {
    728+
    const { parentURL, importAttributes } = context;
    724729
    const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes);
    725730
    const cachedResult = this.#resolveCache.get(requestKey, parentURL);
    726731
    if (cachedResult != null) {
    727732
    return cachedResult;
    728733
    }
    729-
    const result = this.defaultResolve(specifier, parentURL, importAttributes);
    734+
    defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
    735+
    const result = defaultResolve(specifier, context);
    730736
    this.#resolveCache.set(requestKey, parentURL, result);
    731737
    return result;
    732738
    }
    @@ -754,14 +760,15 @@ class ModuleLoader {
    754760
    * This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks
    755761
    * from module.register() which are run in a blocking fashion for it to be synchronous.
    756762
    * @param {string|URL} specifier See {@link resolveSync}.
    757-
    * @param {{ parentURL?: string, importAttributes: ImportAttributes}} context See {@link resolveSync}.
    763+
    * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
    764+
    * See {@link resolveSync}.
    758765
    * @returns {{ format: string, url: string }}
    759766
    */
    760767
    #resolveAndMaybeBlockOnLoaderThread(specifier, context) {
    761768
    if (this.#customizations) {
    762769
    return this.#customizations.resolveSync(specifier, context.parentURL, context.importAttributes);
    763770
    }
    764-
    return this.#cachedDefaultResolve(specifier, context.parentURL, context.importAttributes);
    771+
    return this.#cachedDefaultResolve(specifier, context);
    765772
    }
    766773

    767774
    /**
    @@ -784,26 +791,12 @@ class ModuleLoader {
    784791
    return resolveWithHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
    785792
    this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
    786793
    }
    787-
    return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { parentURL, importAttributes });
    788-
    }
    789-
    790-
    /**
    791-
    * Our `defaultResolve` is synchronous and can be used in both
    792-
    * `resolve` and `resolveSync`. This function is here just to avoid
    793-
    * repeating the same code block twice in those functions.
    794-
    * @returns {string}
    795-
    */
    796-
    defaultResolve(originalSpecifier, parentURL, importAttributes) {
    797-
    defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
    798-
    799-
    const context = {
    794+
    return this.#resolveAndMaybeBlockOnLoaderThread(specifier, {
    800795
    __proto__: null,
    801796
    conditions: this.#defaultConditions,
    802-
    importAttributes,
    803797
    parentURL,
    804-
    };
    805-
    806-
    return defaultResolve(originalSpecifier, context);
    798+
    importAttributes,
    799+
    });
    807800
    }
    808801

    809802
    /**

    ‎lib/internal/modules/helpers.js‎

    Lines changed: 16 additions & 3 deletions
    Original file line numberDiff line numberDiff line change
    @@ -66,6 +66,9 @@ function toRealPath(requestPath) {
    6666

    6767
    /** @type {Set<string>} */
    6868
    let cjsConditions;
    69+
    /** @type {string[]} */
    70+
    let cjsConditionsArray;
    71+
    6972
    /**
    7073
    * Define the conditions that apply to the CommonJS loader.
    7174
    * @returns {void}
    @@ -75,15 +78,17 @@ function initializeCjsConditions() {
    7578
    const noAddons = getOptionValue('--no-addons');
    7679
    const addonConditions = noAddons ? [] : ['node-addons'];
    7780
    // TODO: Use this set when resolving pkg#exports conditions in loader.js.
    78-
    cjsConditions = new SafeSet([
    81+
    cjsConditionsArray = [
    7982
    'require',
    8083
    'node',
    8184
    ...addonConditions,
    8285
    ...userConditions,
    83-
    ]);
    86+
    ];
    8487
    if (getOptionValue('--experimental-require-module')) {
    85-
    cjsConditions.add('module-sync');
    88+
    cjsConditionsArray.push('module-sync');
    8689
    }
    90+
    ObjectFreeze(cjsConditionsArray);
    91+
    cjsConditions = new SafeSet(cjsConditionsArray);
    8792
    }
    8893

    8994
    /**
    @@ -97,6 +102,13 @@ function getCjsConditions() {
    97102
    return cjsConditions;
    98103
    }
    99104

    105+
    function getCjsConditionsArray() {
    106+
    if (cjsConditionsArray === undefined) {
    107+
    initializeCjsConditions();
    108+
    }
    109+
    return cjsConditionsArray;
    110+
    }
    111+
    100112
    /**
    101113
    * Provide one of Node.js' public modules to user code.
    102114
    * @param {string} id - The identifier/specifier of the builtin module to load
    @@ -418,6 +430,7 @@ module.exports = {
    418430
    flushCompileCache,
    419431
    getBuiltinModule,
    420432
    getCjsConditions,
    433+
    getCjsConditionsArray,
    421434
    getCompileCacheDir,
    422435
    initializeCjsConditions,
    423436
    loadBuiltinModule,

    ‎test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs‎

    Lines changed: 2 additions & 0 deletions
    Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

    ‎test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs‎

    Lines changed: 2 additions & 0 deletions
    Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

    ‎test/fixtures/es-modules/custom-condition/node_modules/foo/foo.cjs‎

    Lines changed: 2 additions & 0 deletions
    Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

    ‎test/fixtures/es-modules/custom-condition/node_modules/foo/package.json‎

    Lines changed: 28 additions & 0 deletions
    Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

    ‎test/module-hooks/test-module-hooks-custom-conditions-cjs.js‎

    Lines changed: 57 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -0,0 +1,57 @@
    1+
    // Similar to test-module-hooks-custom-conditions.mjs, but checking the
    2+
    // real require() instead of the re-invented one for imported CJS.
    3+
    'use strict';
    4+
    const common = require('../common');
    5+
    const { registerHooks } = require('node:module');
    6+
    const assert = require('node:assert');
    7+
    const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
    8+
    9+
    (async () => {
    10+
    // Without hooks, the default condition is used.
    11+
    assert.strictEqual(cjs('foo').result, 'default');
    12+
    assert.strictEqual((await esm('foo')).result, 'default');
    13+
    14+
    // Prepending 'foo' to the conditions array in the resolve hook should
    15+
    // allow a CJS to be resolved with that condition.
    16+
    {
    17+
    const hooks = registerHooks({
    18+
    resolve(specifier, context, nextResolve) {
    19+
    assert(Array.isArray(context.conditions));
    20+
    context.conditions = ['foo', ...context.conditions];
    21+
    return nextResolve(specifier, context);
    22+
    },
    23+
    });
    24+
    assert.strictEqual(cjs('foo/second').result, 'foo');
    25+
    assert.strictEqual((await esm('foo/second')).result, 'foo');
    26+
    hooks.deregister();
    27+
    }
    28+
    29+
    // Prepending 'foo-esm' to the conditions array in the resolve hook should
    30+
    // allow a ESM to be resolved with that condition.
    31+
    {
    32+
    const hooks = registerHooks({
    33+
    resolve(specifier, context, nextResolve) {
    34+
    assert(Array.isArray(context.conditions));
    35+
    context.conditions = ['foo-esm', ...context.conditions];
    36+
    return nextResolve(specifier, context);
    37+
    },
    38+
    });
    39+
    assert.strictEqual(cjs('foo/third').result, 'foo-esm');
    40+
    assert.strictEqual((await esm('foo/third')).result, 'foo-esm');
    41+
    hooks.deregister();
    42+
    }
    43+
    44+
    // Duplicating the 'foo' condition in the resolve hook should not change the result.
    45+
    {
    46+
    const hooks = registerHooks({
    47+
    resolve(specifier, context, nextResolve) {
    48+
    assert(Array.isArray(context.conditions));
    49+
    context.conditions = ['foo', ...context.conditions, 'foo'];
    50+
    return nextResolve(specifier, context);
    51+
    },
    52+
    });
    53+
    assert.strictEqual(cjs('foo/fourth').result, 'foo');
    54+
    assert.strictEqual((await esm('foo/fourth')).result, 'foo');
    55+
    hooks.deregister();
    56+
    }
    57+
    })().then(common.mustCall());

    ‎test/module-hooks/test-module-hooks-custom-conditions-special-values.js‎

    Lines changed: 70 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -0,0 +1,70 @@
    1+
    // Check various special values of `conditions` in the context object
    2+
    // when using synchronous module hooks to override the loaders in a
    3+
    // CJS module.
    4+
    'use strict';
    5+
    const common = require('../common');
    6+
    const { registerHooks } = require('node:module');
    7+
    const assert = require('node:assert');
    8+
    const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
    9+
    10+
    (async () => {
    11+
    // Setting it to undefined would lead to the default conditions being used.
    12+
    {
    13+
    const hooks = registerHooks({
    14+
    resolve(specifier, context, nextResolve) {
    15+
    context.conditions = undefined;
    16+
    return nextResolve(specifier, context);
    17+
    },
    18+
    });
    19+
    assert.strictEqual(cjs('foo').result, 'default');
    20+
    assert.strictEqual((await esm('foo')).result, 'default');
    21+
    hooks.deregister();
    22+
    }
    23+
    24+
    // Setting it to an empty array would lead to the default conditions being used.
    25+
    {
    26+
    const hooks = registerHooks({
    27+
    resolve(specifier, context, nextResolve) {
    28+
    context.conditions = [];
    29+
    return nextResolve(specifier, context);
    30+
    },
    31+
    });
    32+
    assert.strictEqual(cjs('foo/second').result, 'default');
    33+
    assert.strictEqual((await esm('foo/second')).result, 'default');
    34+
    hooks.deregister();
    35+
    }
    36+
    37+
    // If the exports have no default export, it should error.
    38+
    {
    39+
    const hooks = registerHooks({
    40+
    resolve(specifier, context, nextResolve) {
    41+
    context.conditions = [];
    42+
    return nextResolve(specifier, context);
    43+
    },
    44+
    });
    45+
    assert.throws(() => cjs('foo/no-default'), {
    46+
    code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
    47+
    });
    48+
    await assert.rejects(esm('foo/no-default'), {
    49+
    code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
    50+
    });
    51+
    hooks.deregister();
    52+
    }
    53+
    54+
    // If the exports have no default export, it should error.
    55+
    {
    56+
    const hooks = registerHooks({
    57+
    resolve(specifier, context, nextResolve) {
    58+
    context.conditions = 'invalid';
    59+
    return nextResolve(specifier, context);
    60+
    },
    61+
    });
    62+
    assert.throws(() => cjs('foo/third'), {
    63+
    code: 'ERR_INVALID_ARG_VALUE',
    64+
    });
    65+
    await assert.rejects(esm('foo/third'), {
    66+
    code: 'ERR_INVALID_ARG_VALUE',
    67+
    });
    68+
    hooks.deregister();
    69+
    }
    70+
    })().then(common.mustCall());

    0 commit comments

    Comments
     (0)
    0