-
Notifications
You must be signed in to change notification settings - Fork 173
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
Comments
@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). |
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 🙁 |
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. |
@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? |
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. |
That PR isn't correct though, it resolves the specifier using |
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. |
@koshic you'd need to clone Corepack locally, patch v8-compile-cache, and build Corepack. You also need a version of 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. |
@aduh95 , at first - thank you for support! ) BTW, it doesn't work:
Why? Because of .cjs, 'import_meta' defined as empty object and esbuild magic can't help us:
PS steps to reproduce it inside corepack repo - add those 2 files to the root:
Setup shims, and run |
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) { |
@aduh95 it works. but
According to docs, it's a 'Script' instance without 'url' property. So I have to write
|
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:
corepack/sources/corepackUtils.ts
Lines 226 to 228 in e8ae337
Or use something more complicated, like additional package.json field.
The text was updated successfully, but these errors were encountered: