nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.46k stars 160 forks source link

v8-compile-cache doesn't allow to use dynamic import in yarn.config.cjs #342

Open koshic opened 8 months ago

koshic commented 8 months ago

Why is this a problem? Newest packages (like globby) shipped only in ESM format, so they can't be used in yarn.config.cjs.

Initial issue from yarn repo - https://github.com/yarnpkg/berry/issues/5987 Root cause - https://github.com/zertosh/v8-compile-cache/issues/41, missed importModuleDynamically callback.

There is no way to disable v8-compile-cache from yarn launched via corepack, users must manually add DISABLE_V8_COMPILE_CACHE=1 to their environments.

I propose to disable v8-compile-cache for yarn like that: https://github.com/nodejs/corepack/blob/e8ae337022cac06457d7928b8ccfd9672370c646/sources/corepackUtils.ts#L226-L228

Or use something more complicated, like additional package.json field.

koshic commented 7 months ago

@arcanis what do you think about that issue? It's not a blocker but really annoying issue which doesn't allow to use corepack + ESM packages in yarn.config.cjs (nice feature allows to automate a lot of work in large monorepos).

arcanis commented 7 months ago

I'm not convinced we should disable v8-compile-cache for everyone using Yarn just for this one specific issue - using v8-compile-cache makes the Yarn startup faster with Corepack than without, which is a compelling feature.

It also feels the wrong layer to fix it. It's too bad the fix isn't getting merged, though 🙁

aduh95 commented 7 months ago

i wuppose we could patch v8-compile-cache in this repo to validate that the proposed fix is indeed fixing the issue, hopefully that would be useful data for upstream reviewers.

koshic commented 7 months ago

@aduh95 it's a good idea. As far I understand, corepack shipped with node is a bundle (dist/lib/corepack.cjs) and contains all v8-compile-cache code. So, IF fix will work (how to test it properly is another question) patched v8-compile-cache may be included in one of the next corepack releases. Am I right?

aduh95 commented 7 months ago

We would probably not release a patched version until the patch has been merged upstream. But if you're able to report whether the fix does indeed fix the problem, we'd be in a better situation because we would have more data on what needs to happen to close this ticket.

merceyz commented 7 months ago

It's too bad the https://github.com/zertosh/v8-compile-cache/pull/47 isn't getting merged, though

That PR isn't correct though, it resolves the specifier using require.resolve instead of import.meta.resolve. Doing it correctly would probably require https://github.com/nodejs/node/pull/51244 to land first.

koshic commented 7 months ago

require nodejs/node#51244 to land first

Landed. What is our next steps - to fix https://github.com/zertosh/v8-compile-cache/pull/47 or to create new one? I can't help so much with coding, but I can test any solution/proposal in my complex environment.

aduh95 commented 7 months ago

@koshic you'd need to clone Corepack locally, patch v8-compile-cache, and build Corepack. You also need a version of node with the mentioned PR, so you need to either build it from source, or download a nightly version. The patch for Corepack is as follow:

diff --git a/v8-compile-cache.js b/v8-compile-cache.js
index 56fd061621ba7b0b3942a5056d357746ba56a6a6..57318ef119826962d30b76c6ebbadc70e7c0bcd2 100644
--- a/v8-compile-cache.js
+++ b/v8-compile-cache.js
@@ -243,6 +243,16 @@ class NativeCompileCache {
       displayErrors: true,
       cachedData: buffer,
       produceCachedData: true,
+      // Allow importing ESM with import() (otherwise throws "A dynamic import callback was not specified.")
+      // See https://nodejs.org/dist/latest-v20.x/docs/api/vm.html#new-vmscriptcode-options
+      // See https://github.com/nodejs/node/blob/3a6a80a4e1ad3fc3f1b181e1c94ecfd0b17e6dd1/test/parallel/test-vm-module-dynamic-import.js#L10
+      importModuleDynamically: (specifier, {url}, importAttributes) => {
+        // Disable cache if script uses dynamic imports (otherwise throws "Invalid host defined options").
+        this._cacheStore.delete(filename);
+
+        // Dynamic import, should be supported by Node.js if importModuleDynamically is called.
+        return import(import.meta.resolve(specifier, url), {with: importAttributes});
+      },
     });

     if (script.cachedDataProduced) {

If that helps, here are the commands you can use on a non-Windows shell to do that:

git clone https://github.com/nodejs/corepack.git
cd corepack
mkdir .yarn/patches
cat > .yarn/patches/v8-compile-cache-npm-2.4.0-5979f8e405.patch <<'EOF'
diff --git a/v8-compile-cache.js b/v8-compile-cache.js
index 56fd061621ba7b0b3942a5056d357746ba56a6a6..57318ef119826962d30b76c6ebbadc70e7c0bcd2 100644
--- a/v8-compile-cache.js
+++ b/v8-compile-cache.js
@@ -243,6 +243,16 @@ class NativeCompileCache {
       displayErrors: true,
       cachedData: buffer,
       produceCachedData: true,
+      // Allow importing ESM with import() (otherwise throws "A dynamic import callback was not specified.")
+      // See https://nodejs.org/dist/latest-v20.x/docs/api/vm.html#new-vmscriptcode-options
+      // See https://github.com/nodejs/node/blob/3a6a80a4e1ad3fc3f1b181e1c94ecfd0b17e6dd1/test/parallel/test-vm-module-dynamic-import.js#L10
+      importModuleDynamically: (specifier, {url}, importAttributes) => {
+        // Disable cache if script uses dynamic imports (otherwise throws "Invalid host defined options").
+        this._cacheStore.delete(filename);
+
+        // Dynamic import, should be supported by Node.js if importModuleDynamically is called.
+        return import(import.meta.resolve(specifier, url), {with: importAttributes});
+      },
     });

     if (script.cachedDataProduced) {
EOF
git apply <<'EOF'
diff --git a/package.json b/package.json
index 4a17d21..ecda8da 100644
--- a/package.json
+++ b/package.json
@@ -50 +50 @@
-    "v8-compile-cache": "^2.3.0",
+    "v8-compile-cache": "patch:v8-compile-cache@npm%3A2.4.0#~/.yarn/patches/v8-compile-cache-npm-2.4.0-5979f8e405.patch",
EOF
corepack yarn
corepack yarn build

Then you can test the newly built version as follow:

/path/to/node-nightly --experimental-import-meta-resolve /path/to/corepack/repo/dist/corepack.js …

And report if you see improvements.

koshic commented 7 months ago

@aduh95 , at first - thank you for support! )

BTW, it doesn't work:

NODE_OPTIONS=--experimental-vm-modules yarn constraints
Type Error: import_meta.resolve is not a function
    at importModuleDynamically (/Users/xxx/.nvm/versions/node/v22.0.0-nightly202402068a41d9b636/lib/node_modules/corepack/dist/lib/corepack.cjs:39583:39)
    at importModuleDynamicallyWrapper (node:internal/vm/module:430:21)
    at importModuleDynamicallyCallback (node:internal/modules/esm/utils:225:14)
    at Object.constraints (/Users/xxx/CCW/main/yarn.config.cjs:14:5)
          // Allow importing ESM with import() (otherwise throws "A dynamic import callback was not specified.")
          // See https://nodejs.org/dist/latest-v20.x/docs/api/vm.html\#new-vmscriptcode-options
          // See https://github.com/nodejs/node/blob/3a6a80a4e1ad3fc3f1b181e1c94ecfd0b17e6dd1/test/parallel/test-vm-module-dynamic-import.js\#L10
          importModuleDynamically: (specifier, { url }, importAttributes) => {
            this._cacheStore.delete(filename);
>>>         return import(import_meta.resolve(specifier, url), { with: importAttributes });
          }

Why? Because of .cjs, 'import_meta' defined as empty object and esbuild magic can't help us:

var import_meta, Module, crypto, fs, path, vm, os2, hasOwnProperty, FileSystemBlobStore, NativeCompileCache;
var init_v8_compile_cache = __esm({
  "../../.yarn/berry/cache/v8-compile-cache-patch-e6773ed6d2-10c0.zip/node_modules/v8-compile-cache/v8-compile-cache.js"() {
    "use strict";
    import_meta = {};
    Module = require("module");
    crypto = require("crypto");

PS steps to reproduce it inside corepack repo - add those 2 files to the root:

// yarn.config.cjs
module.exports = {
  async constraints() {
    await import(`./1.mjs`);
  },
};
// 1.mjs
export const x = 1;

Setup shims, and run NODE_OPTIONS=--experimental-vm-modules yarn constraints

aduh95 commented 7 months ago

Would the following work?

diff --git a/v8-compile-cache.js b/v8-compile-cache.js
index 56fd061621ba7b0b3942a5056d357746ba56a6a6..531225549949ee1412f206b38ba50c8d0c78f7f6 100644
--- a/v8-compile-cache.js
+++ b/v8-compile-cache.js
@@ -243,6 +243,18 @@ class NativeCompileCache {
       displayErrors: true,
       cachedData: buffer,
       produceCachedData: true,
+      // Allow importing ESM with import() (otherwise throws "A dynamic import callback was not specified.")
+      // See https://nodejs.org/dist/latest-v20.x/docs/api/vm.html#new-vmscriptcode-options
+      // See https://github.com/nodejs/node/blob/3a6a80a4e1ad3fc3f1b181e1c94ecfd0b17e6dd1/test/parallel/test-vm-module-dynamic-import.js#L10
+      importModuleDynamically: async (specifier, {url}, importAttributes) => {
+        // Disable cache if script uses dynamic imports (otherwise throws "Invalid host defined options").
+        this._cacheStore.delete(filename);
+
+        const {resolve} = await import('data:text/javascript,export const {resolve} = import.meta')
+
+        // Dynamic import, should be supported by Node.js if importModuleDynamically is called.
+        return import(resolve(specifier, url), {with: importAttributes});
+      },
     });

     if (script.cachedDataProduced) {
koshic commented 7 months ago

No )

image

@aduh95 it works. but {url} is undefined, and the whole second argument is

Script {
  cachedData: <Buffer 0b 06 de c0 d6 27 36 c6 d2 1a 00 00 83 c1 26 77 ff b7 96 e1 88 10 00 00 00 00 00 00 00 00 00 00 01 20 54 01 20 07 b0 60 00 00 00 00 06 00 00 00 01 0c ... 4214 more bytes>,
  cachedDataProduced: true,
  sourceMapURL: undefined
}

According to docs, it's a 'Script' instance without 'url' property. So I have to write

          importModuleDynamically: async (specifier, _, importAttributes) => {
            this._cacheStore.delete(filename);
            const {resolve} = await import('data:text/javascript,export const {resolve} = import.meta')
            const url = require('node:url').pathToFileURL(filename);
            return import(resolve(specifier, url), {with: importAttributes});
          }