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

Yarn PnP compatibility? #10

Closed
bencmbrook opened this issue Nov 22, 2022 · 18 comments
Closed

Yarn PnP compatibility? #10

bencmbrook opened this issue Nov 22, 2022 · 18 comments

Comments

@bencmbrook
Copy link

bencmbrook commented Nov 22, 2022

I believe the issue described here (remarkjs/remark-language-server#6 (comment)) is a result of an assumption that packages are stored in a node_modules directory.

I tried running resolve against my project and got the following error

% yarn node test.mjs
file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/errors.js:322
    Error.captureStackTrace(error)
          ^

Error: Cannot find package 'remark' imported from /<mycwd>/test.mjs
    at new NodeError (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/errors.js:276:5)
    at packageResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1060:9)
    at moduleResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1117:20)
    at defaultResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1266:15)
    at resolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/index.js:29:12)
    at file:///<mycwd>/test.mjs:20:19
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:533:24) {
  code: 'ERR_MODULE_NOT_FOUND'
}

where test.mjs is

import {resolve} from 'import-meta-resolve'

console.log(await resolve('remark', import.meta.url))
  • remark is installed
  • I'm using Yarn PnP, where packages are in .yarn/cache/... rather than node_modules
  • Node 16.17.0, Yarn 3.2.3

A more thorough example:

test2.mjs

import {resolve} from 'import-meta-resolve';
import * as remark from 'remark';
import { createRequire } from 'module';

const require = createRequire(import.meta.url);

console.log('remark:', typeof remark, '\n');
console.log('require.resolve:', require.resolve('remark'), '\n');
console.log('import-meta-resolve:');
console.log(await resolve('remark', import.meta.url));

Yields:

remark: object 

require.resolve: /<mycwd>/.yarn/cache/remark-npm-14.0.2-9e7a75b6a9-a534b6c6bb.zip/node_modules/remark/index.js 

import-meta-resolve:
file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/errors.js:322
    Error.captureStackTrace(error)
          ^

Error: Cannot find package 'remark' imported from /<mycwd>/test.mjs
    at new NodeError (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/errors.js:276:5)
    at packageResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1060:9)
    at moduleResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1117:20)
    at defaultResolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/lib/resolve.js:1266:15)
    at resolve (file:///<mycwd>/.yarn/cache/import-meta-resolve-npm-2.1.0-fcf1208127-4554ea5e2d.zip/node_modules/import-meta-resolve/index.js:29:12)
    at file:///<mycwd>/test.mjs:10:19
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:533:24) {
  code: 'ERR_MODULE_NOT_FOUND'
}
@bencmbrook bencmbrook changed the title Yarn PnP support? Yarn PnP compatibility? Nov 22, 2022
@wooorm
Copy link
Owner

wooorm commented Nov 22, 2022

Can you make a tiny example repo that showcases this, using a modern yarn? I am not a yarn (pnp) user myself. That would likely help me.

@bencmbrook
Copy link
Author

@wooorm For sure - thank you for looking into this.

Here's a minimal reproduction (and the README.md describes reproduction steps) https://github.com/bencmbrook/import-meta-resolve-pnp-repro

@wooorm
Copy link
Owner

wooorm commented Nov 22, 2022

Thanks. Running yarn in that repo does generate node_modules. How do you expect me to install dependencies without node_modules?

@bencmbrook
Copy link
Author

Hm, must be some environment differences - let me dig in on that.

Can you confirm yarn -v logs 3.3.0?

@wooorm
Copy link
Owner

wooorm commented Nov 22, 2022

Yes, 3.3.0. Node 19 or 16.15, then running yarn, both generate a node_modules.

@wooorm
Copy link
Owner

wooorm commented Nov 22, 2022

Managed to reproduce. It does say when running yarn:

➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental

That’s likely it. yarn doesn’t support ESM well.

@bencmbrook
Copy link
Author

Hmm yes that could be it, but I don't think it's because the target package, remark, is ESM. I tested it with a package import that's CJS (swap remark for request) and I get the same error.

@wooorm
Copy link
Owner

wooorm commented Nov 22, 2022

I don’t particularly mean that remark itself is esm. More that this project loads files the way Node.js loads files, when in ESM. It can load CJS fine. But yarn PNP is doing magic things to hook into require calls, to not need ‘node_modules’, those hooks are different on the ESM side

@bencmbrook
Copy link
Author

Ah, yeah I'm tracking.

I just pushed up another test, using the native import.meta.resolve() and it resolves successfully.

If you pull the changes, yarn start runs the script with --experimental-import-meta-resolve

@bencmbrook
Copy link
Author

bencmbrook commented Nov 22, 2022

Digging deeper here... The source of the ERR_MODULE_NOT_FOUND is here

let packageJsonUrl = new URL(
'./node_modules/' + packageName + '/package.json',
base
)

Oddly, the reference implementation also does this? https://github.com/nodejs/node/blob/3c040348fe9c49687a796fbeea3fe7a9bc1251ed/lib/internal/modules/esm/resolve.js#L874-L875

This might be Yarn's magic? https://github.com/yarnpkg/berry/blob/master/packages/yarnpkg-pnp/sources/esm-loader/hooks/resolve.ts

@wooorm
Copy link
Owner

wooorm commented Nov 23, 2022

There may have landed new code to support ESM loaders in the upstream import.meta.resolve algorithm.

I am wondering if Yarn PnP can be solved in your actual use case though.
You will need to call Node.js (or even somehow this package deep down through load-plugin in unified-engine through remark-language-server) with an experimental Node.js flag. And pass Yarns custom loader in. Where would you even do that?

Here’s what Yarn says about Yarn + ESM. My conclusion is different though: Don‘t use Yarn if it doesn’t support ESM/Node’s resolving algorithm.

@wooorm
Copy link
Owner

wooorm commented Nov 23, 2022

As I understand it (I backported recent changes just now), loaders are implemented above resolve: https://github.com/nodejs/node/blob/3ebe7535b456771078368ba72345f8d2e16ce695/lib/internal/modules/esm/loader.js.

I don’t think there’s anything to do in this project.

@bencmbrook
Copy link
Author

Yeah, I think I'm just out of luck with this combo. Thanks again for looking into this.

I am wondering if Yarn PnP can be solved in your actual use case though.

My use-case is specifically getting the remark VSCode extension working in a PnP repo, so I have little control over the Node flags. But I might be able to yarn patch this library (or something else on the load-plugin, unified-engine, remark-language-server chain). Maybe a patch to switch load-plugin to use require.resolve or something.

@bencmbrook
Copy link
Author

I'm gonna close this out. Thanks @wooorm !

@arcanis
Copy link

arcanis commented Feb 28, 2024

As I understand it (I backported recent changes just now), loaders are implemented above resolve: https://github.com/nodejs/node/blob/3ebe7535b456771078368ba72345f8d2e16ce695/lib/internal/modules/esm/loader.js.

It's the other way around - import.meta.resolve must go through loaders. If it wasn't, calling import(import.meta.resolve(...)) wouldn't work, which would be unacceptable. You can easily check it:

import {register} from 'module';

// export async function resolve() {
//   return {shortCircuit: true, url: 'file:///lol'};
// }
register('data:application/javascript;charset=utf-8,export%20async%20function%20resolve%28%29%20%7B%0D%0A%20%20return%20%7BshortCircuit%3A%20true%2C%20url%3A%20%27file%3A%2F%2F%2Flol%27%7D%3B%0D%0A%7D');

console.log(import.meta.resolve('foo'));

Prints

file:///hello

In other words, import-meta-resolve should implement loader support to be on parity with import.meta.resolve, this isn't an issue with PnP itself.

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

It's the other way around - import.meta.resolve must go through loaders

Then where is the code in Node that does that?

I know that it’s still done by the function at import.meta.resolve.
But it’s not in the code Node uses to resolve things.
Similar to how import.meta.url can’t be polyfilled.


In other words, import-meta-resolve should implement loader support to be on parity with import.meta.resolve

Where is the API in Node to find which loaders are currently turned on? I don’t think it exists. I don’t think this can be done.

@arcanis
Copy link

arcanis commented Feb 28, 2024

Where is the API in Node to find which loaders are currently turned on? I don’t think it exists. I don’t think this can be done.

That's tricky indeed - Node.js recently changed the way hooks are registered, so you can't even rely on extracting the --loader values from process.execArgv. Perhaps that should just be listed as a limitation of this package.

With that said, shouldn't the ponyfill delegate to the real import.meta.resolve when running on Node.js versions that expose it unflagged, ie 18.19+ and 20.6+? That would address the issue.

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

Perhaps that should just be listed as a limitation of this package.

It is, readme says no loaders & no env variables, no lots of things. There are tons of (experimental) env variable APIs that change how resolving happens.

Those are interesting, but there are also cases where you (as a package author) don’t want the user’s configuration to change what you resolve. That’s my primary use case. And I believe most people who currently use this’s use case too.

This package also allows to pass conditions for example (and doesn’t inherit the users conditions). To illustrate, it could be used to find a browser version first, and fall back otherwise. Such use cases would not work if the users conditions are used.

So if loaders were to be supported, they should be configurable. Inferring the current loaders/conditions etc could be added too.
But this is sooo much work (also to maintain, it’s lots of code that changes often and has to manually be pulled in). I’m not interested.

That would address the issue.

Then there would be significant differences between when the actual import.meta.resolve is available and when not. As in, Yarn PnP wouldn’t work on old Node. At that point, just use import.meta.resolve?

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

3 participants