From 952c0f7c815335ef1a28dd45c3fbad165fd9b81c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 15 Feb 2025 00:30:24 +0100 Subject: [PATCH] module: allow omitting context in synchronous next hooks This aligns the behavior of synchronous hooks with asynchronous hooks by allowing omission of the context parameter in the invocation of next hooks. The contexts are merged along the chain. --- lib/internal/modules/customization_hooks.js | 29 +++++++++++----- ...t-module-hooks-load-context-merged-esm.mjs | 31 +++++++++++++++++ .../test-module-hooks-load-context-merged.js | 33 ++++++++++++++++++ ...module-hooks-load-context-optional-esm.mjs | 14 ++++++++ ...test-module-hooks-load-context-optional.js | 16 +++++++++ ...odule-hooks-resolve-context-merged-esm.mjs | 32 +++++++++++++++++ ...est-module-hooks-resolve-context-merged.js | 34 +++++++++++++++++++ ...ule-hooks-resolve-context-optional-esm.mjs | 14 ++++++++ ...t-module-hooks-resolve-context-optional.js | 16 +++++++++ 9 files changed, 211 insertions(+), 8 deletions(-) create mode 100644 test/module-hooks/test-module-hooks-load-context-merged-esm.mjs create mode 100644 test/module-hooks/test-module-hooks-load-context-merged.js create mode 100644 test/module-hooks/test-module-hooks-load-context-optional-esm.mjs create mode 100644 test/module-hooks/test-module-hooks-load-context-optional.js create mode 100644 test/module-hooks/test-module-hooks-resolve-context-merged-esm.mjs create mode 100644 test/module-hooks/test-module-hooks-resolve-context-merged.js create mode 100644 test/module-hooks/test-module-hooks-resolve-context-optional-esm.mjs create mode 100644 test/module-hooks/test-module-hooks-resolve-context-optional.js diff --git a/lib/internal/modules/customization_hooks.js b/lib/internal/modules/customization_hooks.js index 9570f52ddc5884..7237318abec96f 100644 --- a/lib/internal/modules/customization_hooks.js +++ b/lib/internal/modules/customization_hooks.js @@ -4,6 +4,7 @@ const { ArrayPrototypeFindIndex, ArrayPrototypePush, ArrayPrototypeSplice, + ObjectAssign, ObjectFreeze, StringPrototypeStartsWith, Symbol, @@ -162,19 +163,31 @@ function convertURLToCJSFilename(url) { * @param {'load'|'resolve'} name Name of the hook in ModuleHooks. * @param {Function} defaultStep The default step in the chain. * @param {Function} validate A function that validates and sanitize the result returned by the chain. - * @returns {Function} */ -function buildHooks(hooks, name, defaultStep, validate) { +function buildHooks(hooks, name, defaultStep, validate, mergedContext) { let lastRunIndex = hooks.length; - function wrapHook(index, userHook, next) { - return function wrappedHook(...args) { + /** + * Helper function to wrap around invocation of user hook or the default step + * in order to fill in missing arguments or check returned results. + * Due to the merging of the context, this must be a closure. + * @param {number} index Index in the chain. Default step is 0, last added hook is 1, + * and so on. + * @param {Function} userHookOrDefault Either the user hook or the default step to invoke. + * @param {Function|undefined} next The next wrapped step. If this is the default step, it's undefined. + * @returns {Function} Wrapped hook or default step. + */ + function wrapHook(index, userHookOrDefault, next) { + return function nextStep(arg0, context) { lastRunIndex = index; - const hookResult = userHook(...args, next); + if (context && context !== mergedContext) { + ObjectAssign(mergedContext, context); + } + const hookResult = userHookOrDefault(arg0, mergedContext, next); if (lastRunIndex > 0 && lastRunIndex === index && !hookResult.shortCircuit) { throw new ERR_INVALID_RETURN_PROPERTY_VALUE('true', name, 'shortCircuit', hookResult.shortCircuit); } - return validate(...args, hookResult); + return validate(arg0, mergedContext, hookResult); }; } const chain = [wrapHook(0, defaultStep)]; @@ -330,7 +343,7 @@ function loadWithHooks(url, originalFormat, importAttributes, conditions, defaul return defaultLoad(url, context); } - const runner = buildHooks(loadHooks, 'load', defaultLoad, validateLoad); + const runner = buildHooks(loadHooks, 'load', defaultLoad, validateLoad, context); const result = runner(url, context); const { source, format } = result; @@ -373,7 +386,7 @@ function resolveWithHooks(specifier, parentURL, importAttributes, conditions, de return defaultResolve(specifier, context); } - const runner = buildHooks(resolveHooks, 'resolve', defaultResolve, validateResolve); + const runner = buildHooks(resolveHooks, 'resolve', defaultResolve, validateResolve, context); return runner(specifier, context); } diff --git a/test/module-hooks/test-module-hooks-load-context-merged-esm.mjs b/test/module-hooks/test-module-hooks-load-context-merged-esm.mjs new file mode 100644 index 00000000000000..babcb213cdab60 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-context-merged-esm.mjs @@ -0,0 +1,31 @@ +// Test that the context parameter will be merged in multiple load hooks. + +import * as common from '../common/index.mjs'; +import assert from 'node:assert'; +import { registerHooks } from 'node:module'; + +const hook1 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 2 and 3. + return nextLoad(url, context); + }, 1), +}); + +const hook2 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 3. + return nextLoad(url); // Omit the context. + }, 1), +}); + +const hook3 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + return nextLoad(url, { testProp: 'custom' }); // Add a custom property + }, 1), +}); + +await import('../fixtures/es-modules/message.mjs'); + +hook3.deregister(); +hook2.deregister(); +hook1.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-context-merged.js b/test/module-hooks/test-module-hooks-load-context-merged.js new file mode 100644 index 00000000000000..139b31fc7ffb89 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-context-merged.js @@ -0,0 +1,33 @@ +'use strict'; + +// Test that the context parameter will be merged in multiple load hooks. + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +const hook1 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 2 and 3. + return nextLoad(url, context); + }, 1), +}); + +const hook2 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 3. + return nextLoad(url); // Omit the context. + }, 1), +}); + +const hook3 = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + return nextLoad(url, { testProp: 'custom' }); // Add a custom property + }, 1), +}); + +require('../fixtures/empty.js'); + +hook3.deregister(); +hook2.deregister(); +hook1.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-context-optional-esm.mjs b/test/module-hooks/test-module-hooks-load-context-optional-esm.mjs new file mode 100644 index 00000000000000..47fce041742f44 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-context-optional-esm.mjs @@ -0,0 +1,14 @@ +// Test that the context parameter can be omitted in the nextLoad invocation. + +import * as common from '../common/index.mjs'; +import { registerHooks } from 'node:module'; + +const hook = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + return nextLoad(url); + }, 1), +}); + +await import('../fixtures/es-modules/message.mjs'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-context-optional.js b/test/module-hooks/test-module-hooks-load-context-optional.js new file mode 100644 index 00000000000000..68287c81660998 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-context-optional.js @@ -0,0 +1,16 @@ +'use strict'; + +// Test that the context parameter can be omitted in the nextLoad invocation. + +const common = require('../common'); +const { registerHooks } = require('module'); + +const hook = registerHooks({ + load: common.mustCall(function(url, context, nextLoad) { + return nextLoad(url); + }, 1), +}); + +require('../fixtures/empty.js'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-context-merged-esm.mjs b/test/module-hooks/test-module-hooks-resolve-context-merged-esm.mjs new file mode 100644 index 00000000000000..b91deb49b00fba --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-context-merged-esm.mjs @@ -0,0 +1,32 @@ +// Test that the context parameter will be merged in multiple resolve hooks. + +import * as common from '../common/index.mjs'; +import assert from 'node:assert'; +import { registerHooks } from 'node:module'; + +const hook1 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 2 and 3. + const result = nextResolve(specifier, context); + return result; + }, 1), +}); + +const hook2 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 3. + return nextResolve(specifier); // Omit the context. + }, 1), +}); + +const hook3 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + return nextResolve(specifier, { testProp: 'custom' }); // Add a custom property + }, 1), +}); + +await import('../fixtures/es-modules/message.mjs'); + +hook3.deregister(); +hook2.deregister(); +hook1.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-context-merged.js b/test/module-hooks/test-module-hooks-resolve-context-merged.js new file mode 100644 index 00000000000000..8bc18fc3d9c30d --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-context-merged.js @@ -0,0 +1,34 @@ +'use strict'; + +// Test that the context parameter will be merged in multiple resolve hooks. + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +const hook1 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 2 and 3. + const result = nextResolve(specifier, context); + return result; + }, 1), +}); + +const hook2 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + assert.strictEqual(context.testProp, 'custom'); // It should be merged from hook 3. + return nextResolve(specifier); // Omit the context. + }, 1), +}); + +const hook3 = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + return nextResolve(specifier, { testProp: 'custom' }); // Add a custom property + }, 1), +}); + +require('../fixtures/empty.js'); + +hook3.deregister(); +hook2.deregister(); +hook1.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-context-optional-esm.mjs b/test/module-hooks/test-module-hooks-resolve-context-optional-esm.mjs new file mode 100644 index 00000000000000..a6d4ad7dac4ced --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-context-optional-esm.mjs @@ -0,0 +1,14 @@ +// Test that the context parameter can be omitted in the nextResolve invocation. + +import * as common from '../common/index.mjs'; +import { registerHooks } from 'node:module'; + +const hook = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + return nextResolve(specifier); + }, 1), +}); + +await import('../fixtures/es-modules/message.mjs'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-resolve-context-optional.js b/test/module-hooks/test-module-hooks-resolve-context-optional.js new file mode 100644 index 00000000000000..ea513bd8b5b5d5 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-context-optional.js @@ -0,0 +1,16 @@ +'use strict'; + +// Test that the context parameter can be omitted in the nextResolve invocation. + +const common = require('../common'); +const { registerHooks } = require('module'); + +const hook = registerHooks({ + resolve: common.mustCall(function(specifier, context, nextResolve) { + return nextResolve(specifier); + }, 1), +}); + +require('../fixtures/empty.js'); + +hook.deregister();