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

Incompatible with yarn P'n'P #23

Closed
JounQin opened this issue Feb 28, 2024 · 12 comments
Closed

Incompatible with yarn P'n'P #23

JounQin opened this issue Feb 28, 2024 · 12 comments

Comments

@JounQin
Copy link

JounQin commented Feb 28, 2024

Same as #10

See also conventional-changelog/commitlint#3936

I also tried yarn unplug:

{
  "dependenciesMeta": {
    "@commitlint/config-conventional": {
      "unplugged": true
    },
    "@commitlint/resolve-extends@19.0.1": {
      "unplugged": true
    },
    "import-meta-resolve": {
      "unplugged": true
    }
  }
}
file:///Users/JounQin/.yarn/berry/cache/@commitlint-cli-npm-19.0.1-8d41b28200-10.zip/node_modules/@commitlint/cli/lib/cli.js:129
        throw err;
        ^

Error: Cannot find module "@commitlint/config-conventional" from "/Users/JounQin/Workspaces/GitHub/test"
    at resolveId (file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:158:17)
    at tryResolveId (file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:145:12)
    at resolveConfig (file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:132:20)
    at file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:96:26
    at Array.reduce (<anonymous>)
    at loadExtends (file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:95:22)
    at resolveExtends (file:///Users/JounQin/Workspaces/GitHub/test/.yarn/unplugged/@commitlint-resolve-extends-npm-19.0.1-78a2e74462/node_modules/@commitlint/resolve-extends/lib/index.js:76:28)
    at load (file:///Users/JounQin/.yarn/berry/cache/@commitlint-load-npm-19.0.1-947ecbb8ee-10.zip/node_modules/@commitlint/load/lib/load.js:44:28)
    at async main (file:///Users/JounQin/.yarn/berry/cache/@commitlint-cli-npm-19.0.1-8d41b28200-10.zip/node_modules/@commitlint/cli/lib/cli.js:202:20) {
  code: 'MODULE_NOT_FOUND'
}

Reproduction: https://github.com/JounQin/test/tree/repro/commitlint

Steps:

  1. yarn install
  2. yarn commitlint --from

Related source codes:

let packageJsonUrl = new URL(
'./node_modules/' + packageName + '/package.json',
base
)
let packageJsonPath = fileURLToPath(packageJsonUrl)
/** @type {string} */
let lastPath
do {
const stat = tryStatSync(packageJsonPath.slice(0, -13))
if (!stat.isDirectory()) {
lastPath = packageJsonPath
packageJsonUrl = new URL(
(isScoped ? '../../../../node_modules/' : '../../../node_modules/') +
packageName +
'/package.json',
packageJsonUrl
)
packageJsonPath = fileURLToPath(packageJsonUrl)
continue
}
// Package match.
const packageConfig = packageJsonReader.read(packageJsonPath, {
base,
specifier
})
if (packageConfig.exports !== undefined && packageConfig.exports !== null) {
return packageExportsResolve(
packageJsonUrl,
packageSubpath,
packageConfig,
base,
conditions
)
}
if (packageSubpath === '.') {
return legacyMainResolve(packageJsonUrl, packageConfig, base)
}
return new URL(packageSubpath, packageJsonUrl)
// Cross-platform root check.
} while (packageJsonPath.length !== lastPath.length)
throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), false)

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

It can’t work, see #10.

@JounQin
Copy link
Author

JounQin commented Feb 28, 2024

Yeah, I looked through that issue, and hope the yarn berry team can help.

cc @arcanis @merceyz @paul-soporan @larixer

@arcanis
Copy link

arcanis commented Feb 28, 2024

This isn't a PnP issue, more a missing import-meta-resolve feature (standard Node.js loader support), see #10 (comment)

@JounQin
Copy link
Author

JounQin commented Feb 28, 2024

I think we'd better discuss in this new issue instead of the closed one.

So I quoted @arcanis's comment here.

As I understand it (I backported recent changes just now), loaders are implemented above resolve: nodejs/node@3ebe753/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.

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

Joun, what you are asking for is that people have to specify --loader some-yarn-loader to your commit-lint CLI.
Do you really want that?

@JounQin
Copy link
Author

JounQin commented Feb 28, 2024

@wooorm I don't think that's the solution? yarn will call its own .pnp.loader.mjs automatically without user action.

image

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

Yes that’s a loader. Someone has to use it?
If you want this package to support loaders like Node does, then it would mean someone has to do commitlint --loader .pnp.loader.mjs?

@JounQin
Copy link
Author

JounQin commented Feb 28, 2024

@wooorm

No, the user don't need to do like that, he/she would only need to run yarn commitlint, no need to specific --loader option. What means yarn wraps --loader .pnp.loader.mjs for the user automatically.

image

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

It sort of does that. It can’t exactly, because your CLI calls Node: https://github.com/conventional-changelog/commitlint/blob/87b1d36d23005b9c1d010ff6a49d1de7f4888cfe/%40commitlint/cli/cli.js.
Yarn is injecting environment variables.
So using Yarn like this means you can’t use node. Jeez.
Is that normal for Yarn users? That they can’t call commitlint? They have to wrap it in yarn commitlint?


Use import.meta.resolve. Don‘t use this package. The readme says that no loaders (and no env variables) are supported: https://github.com/wooorm/import-meta-resolve#differences-to-node.

import.meta.resolve is stable now. Loaders are incredibly complex. You are free to try it, I think it would be another project the size of this one.
There are use cases when you do or don’t want loaders. I like not having to load all that code to support loaders.

@wooorm wooorm closed this as completed Feb 28, 2024
@JounQin
Copy link
Author

JounQin commented Feb 28, 2024

What do you mean by

So using Yarn like this means you can’t use node

There is yarn node

Is that normal for Yarn users? That they can’t call commitlint? They have to wrap it in yarn commitlint?

When use yarn, the user must use yarn commitlint, like npx commitlint, there is no yarn global binaries anymore.


Use import.meta.resolve. Don‘t use this package.

I'd love to, but we're still targeting on Node < 20, and the second parameter is still experimental.

There are use cases when you do or don’t want loaders. I like not having to load all that code to support loaders.

Understand, I'm fine with not supporting loaders(or yarn P'n'P actually) personally.

@wooorm
Copy link
Owner

wooorm commented Feb 28, 2024

I think it’s interesting, the Yarn PnP thing. But it intentionally does lots of things differently. As you show, it has to wrap CLIs and node itself. So it’s not going to work with several tools.

As for supporting loaders, I think it would be a lot of work. Also to maintain. I don’t think anyone is interested in doing that work. See #10 (comment).

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