From 4b2b41305d3b05e1c410e2489ab65cad52dc8411 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Sun, 29 Dec 2024 20:36:54 +0000 Subject: [PATCH 1/8] module: fixing url change in load sync hook chain Fixes: https://github.com/nodejs/node/issues/56376 --- lib/internal/modules/cjs/loader.js | 2 +- .../test-module-hooks-load-url-change.js | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 test/module-hooks/test-module-hooks-load-url-change.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index c453f2b403e89d..d55c7cec101d9b 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1144,7 +1144,7 @@ function getDefaultLoad(url, filename) { return function defaultLoad(urlFromHook, context) { // If the url is the same as the original one, save the conversion. const isLoadingOriginalModule = (urlFromHook === url); - const filenameFromHook = isLoadingOriginalModule ? filename : convertURLToCJSFilename(url); + const filenameFromHook = isLoadingOriginalModule ? filename : convertURLToCJSFilename(urlFromHook); const source = defaultLoadImpl(filenameFromHook, context.format); // Format from context is directly returned, because format detection should only be // done after the entire load chain is completed. diff --git a/test/module-hooks/test-module-hooks-load-url-change.js b/test/module-hooks/test-module-hooks-load-url-change.js new file mode 100644 index 00000000000000..809de73998a60a --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-url-change.js @@ -0,0 +1,29 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); +const fixtures = require('../common/fixtures'); + +// This tests shows the url parameter in `load` can be changed in the `nextLoad` call + +const hook = registerHooks({ + resolve(specifier, context, nextLoad) { + assert.strictEqual(specifier, 'rfs'); + return { + url: 'file:///rfs', + shortCircuit: true, + }; + }, + load: common.mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, 'file:///rfs'); + return nextLoad( + fixtures.fileURL('module-hooks', `redirected-fs.js`).href, + context + ); + }), +}); + +assert.strictEqual(require('rfs').exports_for_test, 'redirected fs'); + +hook.deregister(); From d0e5ef7a4f8e706c0bdfd58653ba5862386ba4b3 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Mon, 30 Dec 2024 21:12:16 +0000 Subject: [PATCH 2/8] more readable test --- test/module-hooks/test-module-hooks-load-url-change.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change.js b/test/module-hooks/test-module-hooks-load-url-change.js index 809de73998a60a..72e2ce8fe1aaa9 100644 --- a/test/module-hooks/test-module-hooks-load-url-change.js +++ b/test/module-hooks/test-module-hooks-load-url-change.js @@ -6,17 +6,18 @@ const { registerHooks } = require('module'); const fixtures = require('../common/fixtures'); // This tests shows the url parameter in `load` can be changed in the `nextLoad` call +// It changes `foo` package name into `redirected-fs` and then loads `redirected-fs` const hook = registerHooks({ resolve(specifier, context, nextLoad) { - assert.strictEqual(specifier, 'rfs'); + assert.strictEqual(specifier, 'foo'); return { - url: 'file:///rfs', + url: 'file:///foo', shortCircuit: true, }; }, load: common.mustCall(function load(url, context, nextLoad) { - assert.strictEqual(url, 'file:///rfs'); + assert.strictEqual(url, 'file:///foo'); return nextLoad( fixtures.fileURL('module-hooks', `redirected-fs.js`).href, context @@ -24,6 +25,6 @@ const hook = registerHooks({ }), }); -assert.strictEqual(require('rfs').exports_for_test, 'redirected fs'); +assert.strictEqual(require('foo').exports_for_test, 'redirected fs'); hook.deregister(); From 30f258f7624c890ec48971895492b6fec3226560 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Thu, 2 Jan 2025 14:59:56 +0000 Subject: [PATCH 3/8] Update test/module-hooks/test-module-hooks-load-url-change.js Co-authored-by: Antoine du Hamel --- test/module-hooks/test-module-hooks-load-url-change.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change.js b/test/module-hooks/test-module-hooks-load-url-change.js index 72e2ce8fe1aaa9..bba39b2860b37d 100644 --- a/test/module-hooks/test-module-hooks-load-url-change.js +++ b/test/module-hooks/test-module-hooks-load-url-change.js @@ -20,7 +20,7 @@ const hook = registerHooks({ assert.strictEqual(url, 'file:///foo'); return nextLoad( fixtures.fileURL('module-hooks', `redirected-fs.js`).href, - context + context, ); }), }); From 9ef1e5015d4c1734ede5203238fad1c542040139 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Mon, 6 Jan 2025 21:17:45 +0000 Subject: [PATCH 4/8] Update test/module-hooks/test-module-hooks-load-url-change.js Co-authored-by: Joyee Cheung --- test/module-hooks/test-module-hooks-load-url-change.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change.js b/test/module-hooks/test-module-hooks-load-url-change.js index bba39b2860b37d..dc68c23ac4c267 100644 --- a/test/module-hooks/test-module-hooks-load-url-change.js +++ b/test/module-hooks/test-module-hooks-load-url-change.js @@ -9,7 +9,7 @@ const fixtures = require('../common/fixtures'); // It changes `foo` package name into `redirected-fs` and then loads `redirected-fs` const hook = registerHooks({ - resolve(specifier, context, nextLoad) { + resolve(specifier, context, nextResolve) { assert.strictEqual(specifier, 'foo'); return { url: 'file:///foo', From 371876f77470e369a40f909aed9ef32638f2298a Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Tue, 7 Jan 2025 17:19:30 +0000 Subject: [PATCH 5/8] url rewriting for import --- ...st-module-hooks-load-url-change-import.mjs | 30 +++++++++++++++++++ ...t-module-hooks-load-url-change-require.js} | 0 2 files changed, 30 insertions(+) create mode 100644 test/module-hooks/test-module-hooks-load-url-change-import.mjs rename test/module-hooks/{test-module-hooks-load-url-change.js => test-module-hooks-load-url-change-require.js} (100%) diff --git a/test/module-hooks/test-module-hooks-load-url-change-import.mjs b/test/module-hooks/test-module-hooks-load-url-change-import.mjs new file mode 100644 index 00000000000000..3489ae83f466b1 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-url-change-import.mjs @@ -0,0 +1,30 @@ +'use strict'; + +import { mustCall } from '../common/index.mjs'; +import assert from 'node:assert'; +import { registerHooks } from 'node:module'; +import { fileURL } from '../common/fixtures.mjs'; + +// This tests shows the url parameter in `load` can be changed in the `nextLoad` call +// It changes `foo` package name into `redirected-fs` and then loads `redirected-fs` + +const hook = registerHooks({ + resolve(specifier, context, nextResolve) { + assert.strictEqual(specifier, 'foo'); + return { + url: 'file:///foo', + shortCircuit: true, + }; + }, + load: mustCall(function load(url, context, nextLoad) { + assert.strictEqual(url, 'file:///foo'); + return nextLoad( + fileURL('module-hooks', `redirected-fs.js`).href, + context + ); + }), +}); + +assert.strictEqual((await import('foo')).exports_for_test, 'redirected fs'); + +hook.deregister(); diff --git a/test/module-hooks/test-module-hooks-load-url-change.js b/test/module-hooks/test-module-hooks-load-url-change-require.js similarity index 100% rename from test/module-hooks/test-module-hooks-load-url-change.js rename to test/module-hooks/test-module-hooks-load-url-change-require.js From 7d63559794178972ef988d20d335ac0eb1e26479 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Wed, 8 Jan 2025 12:01:54 +0000 Subject: [PATCH 6/8] a test of import and a fix for windows --- .../test-module-hooks-load-url-change-import.mjs | 6 ++++-- .../test-module-hooks-load-url-change-require.js | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change-import.mjs b/test/module-hooks/test-module-hooks-load-url-change-import.mjs index 3489ae83f466b1..335db292fb547b 100644 --- a/test/module-hooks/test-module-hooks-load-url-change-import.mjs +++ b/test/module-hooks/test-module-hooks-load-url-change-import.mjs @@ -4,6 +4,8 @@ import { mustCall } from '../common/index.mjs'; import assert from 'node:assert'; import { registerHooks } from 'node:module'; import { fileURL } from '../common/fixtures.mjs'; +import { pathToFileURL } from "node:url"; +import { sep as pathSep } from "node:path"; // This tests shows the url parameter in `load` can be changed in the `nextLoad` call // It changes `foo` package name into `redirected-fs` and then loads `redirected-fs` @@ -12,12 +14,12 @@ const hook = registerHooks({ resolve(specifier, context, nextResolve) { assert.strictEqual(specifier, 'foo'); return { - url: 'file:///foo', + url: 'foo://bar', shortCircuit: true, }; }, load: mustCall(function load(url, context, nextLoad) { - assert.strictEqual(url, 'file:///foo'); + assert.strictEqual(url, 'foo://bar'); return nextLoad( fileURL('module-hooks', `redirected-fs.js`).href, context diff --git a/test/module-hooks/test-module-hooks-load-url-change-require.js b/test/module-hooks/test-module-hooks-load-url-change-require.js index dc68c23ac4c267..3d9f3eb5d79b9b 100644 --- a/test/module-hooks/test-module-hooks-load-url-change-require.js +++ b/test/module-hooks/test-module-hooks-load-url-change-require.js @@ -12,12 +12,12 @@ const hook = registerHooks({ resolve(specifier, context, nextResolve) { assert.strictEqual(specifier, 'foo'); return { - url: 'file:///foo', + url: "foo://bar", shortCircuit: true, }; }, load: common.mustCall(function load(url, context, nextLoad) { - assert.strictEqual(url, 'file:///foo'); + assert.strictEqual(url, "foo://bar",); return nextLoad( fixtures.fileURL('module-hooks', `redirected-fs.js`).href, context, From e73b52c15db0ffe0de5dbd59ce87a11602f79b08 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Thu, 9 Jan 2025 09:16:00 +0000 Subject: [PATCH 7/8] lint fixes --- .../test-module-hooks-load-url-change-import.mjs | 9 +-------- .../test-module-hooks-load-url-change-require.js | 8 ++++---- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change-import.mjs b/test/module-hooks/test-module-hooks-load-url-change-import.mjs index 335db292fb547b..a65975ab1fad16 100644 --- a/test/module-hooks/test-module-hooks-load-url-change-import.mjs +++ b/test/module-hooks/test-module-hooks-load-url-change-import.mjs @@ -1,11 +1,7 @@ -'use strict'; - import { mustCall } from '../common/index.mjs'; import assert from 'node:assert'; import { registerHooks } from 'node:module'; import { fileURL } from '../common/fixtures.mjs'; -import { pathToFileURL } from "node:url"; -import { sep as pathSep } from "node:path"; // This tests shows the url parameter in `load` can be changed in the `nextLoad` call // It changes `foo` package name into `redirected-fs` and then loads `redirected-fs` @@ -20,10 +16,7 @@ const hook = registerHooks({ }, load: mustCall(function load(url, context, nextLoad) { assert.strictEqual(url, 'foo://bar'); - return nextLoad( - fileURL('module-hooks', `redirected-fs.js`).href, - context - ); + return nextLoad(fileURL('module-hooks', 'redirected-fs.js').href, context); }), }); diff --git a/test/module-hooks/test-module-hooks-load-url-change-require.js b/test/module-hooks/test-module-hooks-load-url-change-require.js index 3d9f3eb5d79b9b..9386cab8f4ee04 100644 --- a/test/module-hooks/test-module-hooks-load-url-change-require.js +++ b/test/module-hooks/test-module-hooks-load-url-change-require.js @@ -12,15 +12,15 @@ const hook = registerHooks({ resolve(specifier, context, nextResolve) { assert.strictEqual(specifier, 'foo'); return { - url: "foo://bar", + url: 'foo://bar', shortCircuit: true, }; }, load: common.mustCall(function load(url, context, nextLoad) { - assert.strictEqual(url, "foo://bar",); + assert.strictEqual(url, 'foo://bar'); return nextLoad( - fixtures.fileURL('module-hooks', `redirected-fs.js`).href, - context, + fixtures.fileURL('module-hooks', 'redirected-fs.js').href, + context ); }), }); From 406b3d26b4ea0a0750a485971405d1e3ba4c5e31 Mon Sep 17 00:00:00 2001 From: Vitalii Akimov Date: Fri, 10 Jan 2025 18:29:24 +0000 Subject: [PATCH 8/8] linter fix --- test/module-hooks/test-module-hooks-load-url-change-require.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/module-hooks/test-module-hooks-load-url-change-require.js b/test/module-hooks/test-module-hooks-load-url-change-require.js index 9386cab8f4ee04..7f10110cda8c50 100644 --- a/test/module-hooks/test-module-hooks-load-url-change-require.js +++ b/test/module-hooks/test-module-hooks-load-url-change-require.js @@ -20,7 +20,7 @@ const hook = registerHooks({ assert.strictEqual(url, 'foo://bar'); return nextLoad( fixtures.fileURL('module-hooks', 'redirected-fs.js').href, - context + context, ); }), });