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

[Bug?]: Yarn pnp / sdk generation doesn't work with ESLint new flat config support #6219

Closed
1 task
dbaeumer opened this issue Apr 11, 2024 · 25 comments
Closed
1 task
Labels
bug Something isn't working

Comments

@dbaeumer
Copy link

dbaeumer commented Apr 11, 2024

Self-service

  • I'd be willing to implement a fix

Describe the bug

ESLint's new flat config support fails to load when using flat config module files. It does work when using common JS config files.

This bahviour gor reported against the VS Code ESLint extension (see microsoft/vscode-eslint#1620) but it is independent of the VS Code extension and can be reproduce using the eslint npm package standalone

To reproduce

Setup:

package.json

{
  "name": "yarn-eslint-test",
  "packageManager": "yarn@4.1.1",
  "type": "module",
  "scripts": {
    "eslint": "eslint test.js"
  },
  "devDependencies": {
    "@eslint/js": "^8.57.0",
    "eslint": "^8.57.0"
  }
}

eslint.config.mjs

import js from "@eslint/js";

export default [
	js.configs.recommended,
	{
		files: ["**/*.js"],
		rules: {
			"no-var": "error",
		}
	}
];

test.js

var x = 10;

Run the following commands in a shell:

> corepack enable
> yarn install
> yarn dlx @yarnpkg/sdks vscode
> export NODE_PATH=".yarn/sdks"

Setting the NODE_PATH is equivalent to setting the VS Code eslint.nodePath setting.

start node REPL and execute the following commands (to ensure to mimic the executing since a VS Code extension):

require.resolve('eslint');

We will load the eslint npm module from .yarn/sdks

const library = require('eslint');
async function main() { const eslint = new (await library.loadESLint({ useFlatConfig: true }))(); const report = await eslint.lintText(`var test = "hello";`); console.log(report); }
main().catch((error) => { console.error(error); process.exitCode = 1; });

You get:

Image

Expected behavior: the eslint.config.mjs can successfully be loaded. Converting the config file to cjs makes everything work.

Environment

System:
OS: Linux 5.15 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700K
Binaries:
Node: 18.18.2 - /tmp/xfs-e72dc863/node
Yarn: 4.1.1 - /tmp/xfs-e72dc863/yarn
npm: 9.8.1 - ~/.nvs/default/bin/npm

Additional context

No response

@dbaeumer dbaeumer added the bug Something isn't working label Apr 11, 2024
@dbaeumer dbaeumer changed the title [Bug?]: Yarn pnp / sdk generation doesn;t work with ESLint new flat config support [Bug?]: Yarn pnp / sdk generation doesn't work with ESLint new flat config support Apr 11, 2024
qwell added a commit to qwell/symbol.wtf that referenced this issue Apr 20, 2024
@dbaeumer
Copy link
Author

dbaeumer commented May 6, 2024

Any news on this. This issue is basically blocking flat config adaption with VS Code / eslint extension.

@arcanis
Copy link
Member

arcanis commented May 6, 2024

I'm looking right now at migrating this repository to Eslint 9; I'll take a look at that at the same time.

@arcanis
Copy link
Member

arcanis commented May 6, 2024

I'm experimenting in #6278 - it seems to work with your (very detailed, thanks a lot!) reproduction, but it still fails in VSCode. Trying to figure out what might be different.

@arcanis
Copy link
Member

arcanis commented May 6, 2024

I think I figured it out - v18.19.0 is the first version that supports register (to add ESM loaders at runtime), and VSCode ships with Electron 28, which is tied to Node 18.18. So unfortunately I'm afraid we can't do much right now 🫤

@dbaeumer
Copy link
Author

dbaeumer commented May 7, 2024

@arcanis thanks for looking into this.

Which node version is required to make this work. I tested the same steps under Node 20.10.0 and it produces the same exception.

Image

@arcanis
Copy link
Member

arcanis commented May 7, 2024

You need both my PR (#6278) and Node >v20.6.0, v18.19.0; you can try it by cloning my branch and running your repro in this repo.

@dbaeumer
Copy link
Author

dbaeumer commented May 7, 2024

Thanks!

We will move to Electron 29 in the near future which will come with NodeJS 20.x

@dbaeumer
Copy link
Author

@arcanis do you have any timeline when this will show up in yarn dlx?

@arcanis
Copy link
Member

arcanis commented May 21, 2024

I'd like to confirm it works before merging it - is there a vscode build with an up-to-date Node.js?

@clemyan
Copy link
Member

clemyan commented May 22, 2024

is there a vscode build with an up-to-date Node.js?

The insiders build has updated to Electron 29 with Node 20.9

@dbaeumer
Copy link
Author

@clemyan thanks for point it out.

@arcanis you can find the insider build here: https://code.visualstudio.com/insiders/

@n0099
Copy link
Contributor

n0099 commented Jun 6, 2024

#6219 (comment)

We will move to Electron 29 in the near future which will come with NodeJS 20.x

https://code.visualstudio.com/updates/v1_90#_electron-29-update

#6219 (comment)

You need both my PR (#6278) and Node >v20.6.0, v18.19.0; you can try it by cloning my branch and running your repro in this repo.

I can confirm this still won't work with the extension dbaeumer.vscode-eslint@3.0.7 on both vscode@1.89 with node@18.18 or vscode@1.90 and vscode-insider@1.91 with node@20.9, or even using my system wide node@22.2 via the extension setting eslint.runtime

@arcanis
Copy link
Member

arcanis commented Jun 6, 2024

@n0099 did you try with #6278? I just tried and this PR, together with vscode-insider, seems to work fine. I'll look at making a final merge and release soon.

arcanis added a commit that referenced this issue Jun 6, 2024
## What's the problem this PR addresses?

ESM loaders aren't supported by the sdks because we only
`require().setup()` but we don't `register()` the ESM loader hook.

Fixes #6219

## How did you fix it?

- Add support for ESM loaders to the SDK.

- Check whether the `findPnpApi` function could be found in the `module`
builtin. If not, tries to setup the environment accordingly. This is
because in the case of the SDK, the subprocesses have been created
without the `--require .pnp.loader.mjs` flag, so the PnP runtime won't
be setup.

## Checklist

<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@Sebbones
Copy link

Sebbones commented Jun 6, 2024

I can confirm the same as @n0099. I still get ERR_MODULE_NOT_FOUND using multiple setups with #6278 and node@20.9:

  • vscode-insider@1.91, dbaeumer.vscode-eslint@3.0.7, eslint@^9.4.0 - ❌
  • vscode-insider@1.91, dbaeumer.vscode-eslint@3.0.7, eslint@^8.57.0 - ❌
  • vscode-insider@1.91, dbaeumer.vscode-eslint@2.4.4 (eslint.experimental.useFlatConfig: true), eslint@^9.4.0 - ❌
  • vscode-insider@1.91, dbaeumer.vscode-eslint@2.4.4 (eslint.experimental.useFlatConfig: true), eslint@^8.57.0 - ❌

I debugged a little and it is still unable to find findPnpApi() in the resolve hook for me when executed in the extension. Cli execution works fine.

Maybe i am missing something in my setup?

Happy to provide more info if needed 😊

@n0099
Copy link
Contributor

n0099 commented Jun 6, 2024

@n0099 did you try with #6278?

Yes.

  • vscode-insider@1.91, dbaeumer.vscode-eslint@2.4.4 (eslint.experimental.useFlatConfig: true), eslint@^9.4.0 - ❌

I've also tried using old extension dbaeumer.vscode-eslint@2.4.4 with eslint.experimental.useFlatConfig: true, but not with older eslint@^8.

Cli execution works fine.

btw did you did yarn unplug eslint before? For me cli can only be used with .yarn/unplugged/eslint-npm-*/node_modules/eslint/bin/eslint.js

@Sebbones
Copy link

Sebbones commented Jun 6, 2024

btw did you did yarn unplug eslint before? For me cli can only be used with .yarn/unplugged/eslint-npm-*/node_modules/eslint/bin/eslint.js

No, i works with the 'plugged' version for me.

@n0099
Copy link
Contributor

n0099 commented Jun 6, 2024

I'm using tsx to run eslint: pzmosquito/eslint-import-resolver-vite#12 (comment), so it was required by then, but with flat config now it can get re-plugged: pzmosquito/eslint-import-resolver-vite#12 (comment)

@Sebbones
Copy link

Sebbones commented Jun 7, 2024

I can reproduce this without the extension using the sdk version from #6278 and running

node --import ./.yarn/sdks/eslint/lib/api.js ./eslint.config.mjs ❌

node@22.2.0

If i run the config through yarn node it can resolve all modules.

yarn node ./eslint.config.mjs ✅

UPDATE
It works, i only installed the updated sdk. But i needed to build the full cli so the .pnp.loader.mjs gets updated aswell.

@n0099 Do not use yarn dlx -p '@yarnpkg/sdks@https://github.com/yarnpkg/berry#head=mael/eslint-flat-config&workspace=@yarnpkg/sdks' @yarnpkg/sdks vscode
Instead build the cli from the correct branch https://github.com/yarnpkg/berry/tree/mael/eslint-flat-config (yarn build:cli) yourself and link the new cli in your .yarnrc.yml with yarnPath. It should work.

@CaseyHofland
Copy link

@Sebbones I'm implementing this on Wednesday and I find your findings really helpful. There's a bit of clutter now, could I ask you what steps you'd suggest to correctly install this in a clean project?

@n0099
Copy link
Contributor

n0099 commented Jun 10, 2024

Instead build the cli from the correct branch mael/eslint-flat-config (yarn build:cli) yourself and link the new cli in your .yarnrc.yml with yarnPath. It should work.

could I ask you what steps you'd suggest to correctly install this in a clean project?

#6278 is already merged 4 days ago, so just running yarn dlx @yarnpkg/sdks vscode should be enough by now.

@IvanHoliak
Copy link

Is this not fixed yet? because I still have the same problem( Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/js' imported from
node v22.1.0
yarn v4.2.2
eslint v9.4.0

@n0099
Copy link
Contributor

n0099 commented Jun 10, 2024

yarn dlx @yarnpkg/sdks vscode should be enough by now.

Until a new version of @yarnpkg/cli is released, we may only do as in #6219 (comment):

Instead build the cli from the correct branch mael/eslint-flat-config (yarn build:cli) yourself and link the new cli in your .yarnrc.yml with yarnPath. It should work.

Steps:

your_project_root=
git clone --filter=tree:0 https://github.com/yarnpkg/berry yarn
cd yarn
git checkout 0cef81fe1bdc35def2c6d0e88dd44a998707661c
yarn build:cli
cp packages/yarnpkg-cli/bundles/yarn.js $your_project_root/.yarn/releases/yarn-0cef81fe.js
cd $your_project_root
sed -i 's@yarnPath: .yarn/releases/yarn-4.2.2.cjs@yarnPath: .yarn/releases/yarn-0cef81fe.js@' .yarnrc.yml
yarn dlx @yarnpkg/sdks vscode

Then I get #5831:

Error: Dynamic require of "util" is not supported
    at file:///.yarn/releases/yarn-0cef81fe.js:4:394
    at file:///.yarn/releases/yarn-0cef81fe.js:4:3572
    at file:///.yarn/releases/yarn-0cef81fe.js:4:478
    at file:///.yarn/releases/yarn-0cef81fe.js:9:46377
    at file:///.yarn/releases/yarn-0cef81fe.js:4:478
    at file:///.yarn/releases/yarn-0cef81fe.js:392:1509
    at file:///.yarn/releases/yarn-0cef81fe.js:735:11937
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

@arcanis
Copy link
Member

arcanis commented Jun 10, 2024

I just triggered a release. Using the 4.3.0 the original test case doesn't crash anymore. In case you still face issues, please make sure you use both Yarn 4.3 and the SDK 3.1.3. If you do, open a new thread (let's not reuse this one, I'd like to keep investigation separate as something that may look related may be a completely different bug).

For reference, here's a script to easily run the commands from the op; note I updated packageManager to 4.3:

#!/usr/bin/env bash

cd $(mktemp -d)

cat > package.json <<EOF
{
  "name": "yarn-eslint-test",
  "packageManager": "yarn@4.3.0",
  "type": "module",
  "scripts": {
    "eslint": "eslint test.js"
  },
  "devDependencies": {
    "@eslint/js": "^8.57.0",
    "eslint": "^8.57.0"
  }
}
EOF

cat > eslint.config.mjs <<EOF
import js from "@eslint/js";

export default [
	js.configs.recommended,
	{
		files: ["**/*.js"],
		rules: {
			"no-var": "error",
		}
	}
];
EOF

cat > test.js <<EOF
var x = 10;
EOF

yarn install
yarn dlx @yarnpkg/sdks vscode
export NODE_PATH=".yarn/sdks"

node <<EOF
require.resolve('eslint');

const library = require('eslint');
async function main() { const eslint = new (await library.loadESLint({ useFlatConfig: true }))(); const report = await eslint.lintText('var test = "hello";'); console.log(report); }
main().catch((error) => { console.error(error); process.exitCode = 1; });
EOF

@arcanis arcanis closed this as completed Jun 10, 2024
n0099 added a commit to n0099/open-tbm that referenced this issue Jun 10, 2024
$ yarn add -D @typescript-eslint/{parser,eslint-plugin}@npm:rc-v8
$ yarn dedupe
$ yarn set version latest # fix yarnpkg/berry#6219
* remove `yarnPath` to prefer corepack @ .yarnrc.yml
@ fe
n0099 added a commit to n0099/open-tbm that referenced this issue Jun 10, 2024
$ yarn add -D @typescript-eslint/{parser,eslint-plugin}@npm:rc-v8
$ yarn dedupe
$ yarn set version latest # fix yarnpkg/berry#6219
* remove `yarnPath` to prefer corepack @ .yarnrc.yml
@ fe

* run `corepack enable` to fix actions/setup-node#1027
@ .github/actions/fe/action.yml
@kherock
Copy link
Contributor

kherock commented Jun 26, 2024

@arcanis is this register behavior something that libraries are expected to implement when supporting PnP? Or should a stylelint SDK be re-added to help out with the vscode-stylelint library?

@psychobolt
Copy link

psychobolt commented Jul 9, 2024

In the past I think there was a vscode stylelint SDK. I don't see why not since the ecosystem supports it. But given of what we know, it seems fair to notify the plugin devs for a complete solution on their end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants