Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
koshic opened this issue Dec 22, 2023 · 11 comments
Open

Comments

@koshic
Copy link

koshic commented Dec 22, 2023

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 - yarnpkg/berry#5987
Root cause - zertosh/v8-compile-cache#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:

if (locator.name !== `npm` || semver.lt(locator.reference, `9.7.0`))
// @ts-expect-error - No types
await import(`v8-compile-cache`);

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

@koshic
Copy link
Author

koshic commented Jan 22, 2024

@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
Copy link
Contributor

arcanis commented Jan 23, 2024

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
Copy link
Contributor

aduh95 commented Jan 23, 2024

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
Copy link
Author

koshic commented Jan 23, 2024

@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
Copy link
Contributor

aduh95 commented Jan 23, 2024

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
Copy link
Member

merceyz commented Jan 23, 2024

It's too bad the zertosh/v8-compile-cache#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 nodejs/node#51244 to land first.

@koshic
Copy link
Author

koshic commented Feb 6, 2024

require nodejs/node#51244 to land first

Landed. What is our next steps - to fix zertosh/v8-compile-cache#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
Copy link
Contributor

aduh95 commented Feb 6, 2024

@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
Copy link
Author

koshic commented Feb 6, 2024

@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
Copy link
Contributor

aduh95 commented Feb 6, 2024

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
Copy link
Author

koshic commented Feb 6, 2024

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});
          }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants