From f5a07d2e8779d3936b2d04bc7b47ee35172c9a8f Mon Sep 17 00:00:00 2001 From: Laurent Christophe Date: Fri, 7 Oct 2022 10:19:35 +0200 Subject: [PATCH] refactor: improve main module loading resolution Instead of rewritting main files we simply add some logic in then loader -- cf: https://github.com/nodejs/node/issues/41465 A la tchusss abomination --- components/.ordering | 1 - components/abomination/default/.env | 1 - components/abomination/default/index.mjs | 63 ------------------- components/abomination/default/index.test.mjs | 22 ------- .../configuration-accessor/default/index.mjs | 6 -- .../default/index.test.mjs | 2 - lib/node/abomination.js | 33 ---------- lib/node/loader.mjs | 28 +++++++++ 8 files changed, 28 insertions(+), 128 deletions(-) delete mode 100644 components/abomination/default/.env delete mode 100644 components/abomination/default/index.mjs delete mode 100644 components/abomination/default/index.test.mjs delete mode 100644 lib/node/abomination.js diff --git a/components/.ordering b/components/.ordering index 87c3a3016..f1ba44aa5 100644 --- a/components/.ordering +++ b/components/.ordering @@ -59,6 +59,5 @@ receptor batch questionnaire setup -abomination init status diff --git a/components/abomination/default/.env b/components/abomination/default/.env deleted file mode 100644 index 64f5a0a68..000000000 --- a/components/abomination/default/.env +++ /dev/null @@ -1 +0,0 @@ -node diff --git a/components/abomination/default/index.mjs b/components/abomination/default/index.mjs deleted file mode 100644 index 3c3b8de06..000000000 --- a/components/abomination/default/index.mjs +++ /dev/null @@ -1,63 +0,0 @@ -const { URL } = globalThis; - -const { search: __search } = new URL(import.meta.url); - -// https://github.com/mochajs/mocha/issues/4720 - -// This abomination solves the empty file extension issue caused by --experimental-loader. -// Indeed, this flag (seems to) cause the default loader to go from cjs to esm. -// And file without extension works fine with the cjs loader but not with the esm loader. - -import { - readlink as readLinkAsync, - copyFile as copyFileAsync, - unlink as unlinkAsync, - symlink as symlinkAsync, -} from "fs/promises"; -import { - extname as getExtension, - dirname as getDirectory, - resolve as resolvePath, -} from "path"; -const { logInfo } = await import(`../../log/index.mjs${__search}`); -const { expectSuccessAsync, expect } = await import( - `../../expect/index.mjs${__search}` -); - -const readMaybeLinkAsync = async (link) => { - try { - return await readLinkAsync(link); - } catch { - return null; - } -}; - -export const addLinkExtensionAsync = async (link) => { - const path = await readMaybeLinkAsync(link); - if (path === null) { - const extension = getExtension(link); - expect( - extension !== "", - "Cannot solve the empty extension issue because %j is not a symbolic link", - link, - ); - } else { - const extension = getExtension(path); - if (extension === "") { - logInfo( - "Symbolic link %j refers to %j which has no extension, this is problematic for the node's esm loader which is enabled by --experimental-loader. To solve this issue, the agent is going to create copy of that file with a '.cjs' extension and overwrite the link to refer to that file. We hope this is okay with you...", - link, - path, - ); - const absolute_path = resolvePath(getDirectory(link), path); - await expectSuccessAsync( - (async () => { - await copyFileAsync(absolute_path, `${absolute_path}.cjs`); - await unlinkAsync(link); - await symlinkAsync(`${path}.cjs`, link, "file"); - })(), - "Something went wrong when resolving the missing file extension issue >> %O", - ); - } - } -}; diff --git a/components/abomination/default/index.test.mjs b/components/abomination/default/index.test.mjs deleted file mode 100644 index 50ea64793..000000000 --- a/components/abomination/default/index.test.mjs +++ /dev/null @@ -1,22 +0,0 @@ -import { - writeFile as writeFileAsync, - readFile as readFileAsync, - symlink as symlinkAsync, - readlink as readLinkAsync, -} from "fs/promises"; -import { fileURLToPath } from "url"; -import { getFreshTemporaryURL, assertEqual } from "../../__fixture__.mjs"; -import { basename as getBasename } from "path"; -import { addLinkExtensionAsync } from "./index.mjs?env=test"; - -const path1 = fileURLToPath(getFreshTemporaryURL()); -const path2 = fileURLToPath(getFreshTemporaryURL()); - -await writeFileAsync(path1, "123;", "utf8"); -await symlinkAsync(getBasename(path1), path2, "file"); - -await addLinkExtensionAsync(path2); -assertEqual(await readLinkAsync(path2), `${getBasename(path1)}.cjs`); -assertEqual(await readFileAsync(`${path1}.cjs`, "utf8"), "123;"); - -await addLinkExtensionAsync(`${path1}.cjs`); diff --git a/components/configuration-accessor/default/index.mjs b/components/configuration-accessor/default/index.mjs index da440351a..c1a09651b 100644 --- a/components/configuration-accessor/default/index.mjs +++ b/components/configuration-accessor/default/index.mjs @@ -231,12 +231,6 @@ export const compileConfigurationCommand = (configuration, env) => { ...env, NODE_OPTIONS: [ coalesce(env, "NODE_OPTIONS", ""), - // abomination: https://github.com/mochajs/mocha/issues/4720 - `--require=${pathifyURL( - appendURLSegmentArray(directory, ["lib", "node", "abomination.js"]), - base, - true, - )}`, `--experimental-loader=${generateEscape(exec)( pathifyURL( appendURLSegmentArray(directory, [ diff --git a/components/configuration-accessor/default/index.test.mjs b/components/configuration-accessor/default/index.test.mjs index b914afe28..a78b3833a 100644 --- a/components/configuration-accessor/default/index.test.mjs +++ b/components/configuration-accessor/default/index.test.mjs @@ -315,7 +315,6 @@ assertDeepEqual( env: { NODE_OPTIONS: [ "--node-key=node-value", - "--require=../agent/lib/node/abomination.js", "--experimental-loader=../agent/lib/node/recorder-process.mjs", ].join(" "), VAR1: "VAL1", @@ -400,7 +399,6 @@ assertDeepEqual( env: { NODE_OPTIONS: [ "", - "--require=../agent/lib/node/abomination.js", "--experimental-loader=../agent/lib/node/mocha-loader.mjs", ].join(" "), }, diff --git a/lib/node/abomination.js b/lib/node/abomination.js deleted file mode 100644 index feb58ff79..000000000 --- a/lib/node/abomination.js +++ /dev/null @@ -1,33 +0,0 @@ -/* globals require: readonly */ -/* eslint local/no-globals: ["error", "globalThis", "require"] */ - -const { renameSync, realpathSync, existsSync, symlinkSync } = require("fs"); -const { - extname: getExtension, - dirname: getDirectory, - basename: getBasename, - join: joinPath, -} = require("path"); - -const { - Error, - process: { argv }, -} = globalThis; - -// https://github.com/nodejs/node/blob/2cc7a91a5d855b4ff78f21f1bb8d4e55131d0615/lib/internal/modules/run_main.js -// TODO this prevents the original runMain to suppport --preserve-symlinks-main - -const path = realpathSync(argv[1]); -const directory = getDirectory(path); -const extension = getExtension(path); -const basename = getBasename(path, extension); -if (extension === "" || extension === ".node") { - const modified_path = joinPath(directory, `${basename}.cjs`); - if (existsSync(modified_path)) { - throw new Error( - "Unable to solve the file extension issue of ESM loader -- see: https://github.com/nodejs/node/issues/41465", - ); - } - renameSync(path, modified_path); - symlinkSync(modified_path, path, "file"); -} diff --git a/lib/node/loader.mjs b/lib/node/loader.mjs index 077bdaa64..1f5e27966 100644 --- a/lib/node/loader.mjs +++ b/lib/node/loader.mjs @@ -1,4 +1,5 @@ const { + process: { stdout }, Error, undefined, parseInt, @@ -18,9 +19,29 @@ const parseMajorVersion = (version) => { } }; +let is_main = true; + +const message = "Threating main file as commonjs, this might not always be appropriate -- cf: https://github.com/nodejs/node/issues/41465\n"; + export default (node_version, esm_hook_variable) => { const major_node_version = parseMajorVersion(node_version); return { + getFormat: + major_node_version >= 16 + ? undefined + : (url, context, next) => { + if (is_main) { + is_main = false; + try { + return next(url, context, next); + } catch { + stdout.write(message); + return "commonjs"; + } + } else { + return next(url, context, next); + } + }, transformSource: major_node_version >= 16 ? undefined @@ -39,6 +60,13 @@ export default (node_version, esm_hook_variable) => { major_node_version < 16 ? undefined : (url, context, next) => { + if (is_main) { + is_main = false; + if (context.format === undefined) { + stdout.write(message); + context.format = "commonjs"; + } + } if (hasOwn(globalThis, esm_hook_variable)) { return globalThis[esm_hook_variable].load(url, context, next); } else {