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

feat: support for eslint v9 #353

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yutak23
Copy link

@yutak23 yutak23 commented Sep 27, 2024

This ought to fix #351.

ESLint v9 and seems to have moved completely to flat config. Therefore, I believe it is no longer possible to maintain backward compatibility. Therefore, i think it is better to raise the version to 5.0.0 with this modification and release 5.0.0 as supporting only ESLint v9 or higher.
I have tried to support both v8 and v9.
I believe that Node.js support is fine for 17+ to match EOL.

About mocking in rule-finder testing

In calculateConfigArray at https://github.com/eslint/eslint/blob/v9.11.1/lib/eslint/eslint.js#L444,
there is a part where it does new FlatConfigArray,
As per the implementation at https://github.com/eslint/eslint/blob/v9.11.1/lib/config/flat-config-array.js#L90:

constructor(configs, {
        basePath,
        shouldIgnore = true,
        baseConfig = defaultConfig
    } = {}) {

The baseConfig ends up being set to the default config (https://github.com/eslint/eslint/blob/v9.11.1/lib/config/default-config.js).
Therefore, in the following implementation section:

this[originalBaseConfig] = baseConfig;
Object.defineProperty(this, originalBaseConfig, { writable: false });

the following settings inevitably get included:

plugins: {
            "@": {...}
}

and it's not possible to execute calculateConfigForFile with configurations for rules that are not in ESLint.
Therefore, as a workaround, I used a mock to allow initialization with rules that are not in ESLint.
I believe that this does not prevent us from testing what needs to be tested.

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2024

If this project only supports eslint 9 - and not 8 and 9 - it will disrupt the airbnb config from being able to ship eslint 9 support. Separately, I'm not sure why it must be a breaking change. I think we should try to support both simultaneously.

@yutak23
Copy link
Author

yutak23 commented Sep 27, 2024

If this project only supports eslint 9 - and not 8 and 9 - it will disrupt the airbnb config from being able to ship eslint 9 support. Separately, I'm not sure why it must be a breaking change. I think we should try to support both simultaneously.

@ljharb , thank you for your comment.
i think,
For example, many libraries have decided to support only ESMs, but I believe that users of those libraries are continuing to use the versions before ESM-only support.
In that sense, this update is not a problem, since the old version will not disappear, but will continue to be available. For that reason, I propose to raise the major version. What do you think?

As for airbnb, that is exactly what I was working on trying to migrate to 9, but was unable to complete the migration process to 9 because of its dependency on this library. The test before PR does not pass.

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2024

You're correct that most users of libraries that have gone ESM-only are staying on pre-ESM-only versions - but that's not ok, that's a MASSIVE security problem for the entire JS ecosystem. It's an argument against breaking changes.

The airbnb config's next version will support both 8 and 9, and may not even ship a flat config. This tool has to work on both eslintrc and flat config, and eslint 8 and 9, simultaneously.

@yutak23
Copy link
Author

yutak23 commented Sep 27, 2024

You're correct that most users of libraries that have gone ESM-only are staying on pre-ESM-only versions - but that's not ok, that's a MASSIVE security problem for the entire JS ecosystem. It's an argument against breaking changes.

The airbnb config's next version will support both 8 and 9, and may not even ship a flat config. This tool has to work on both eslintrc and flat config, and eslint 8 and 9, simultaneously.

I understand and agree with your thinking.
Now I will try to modify it to support both 8 and 9.

@yutak23 yutak23 changed the title feat: migrate to eslint v9 feat: support for eslint v9 Sep 29, 2024
@yutak23
Copy link
Author

yutak23 commented Sep 29, 2024

@ljharb , I have tried to support both v8 and v9.
I would appreciate your confirmation.

@lotmek
Copy link
Contributor

lotmek commented Sep 29, 2024

Hello @yutak23,

I've tried a similar approach as yours, but I was facing some surprises in the output. Some plugins are not properly imported by the ESLint config object and I don't know why...

You can take a look at this example where I've forked your yutak23:feature/migrate-to-v9 branch in my local, run the build script and added the dependency in my project.

You can see that the eslint-find-rules -p script is not listing the plugin rules for eslint-plugin-json

image

I'm going to investigate another approach

@yutak23
Copy link
Author

yutak23 commented Sep 29, 2024

Hello @yutak23,

I've tried a similar approach as yours, but I was facing some surprises in the output. Some plugins are not properly imported by the ESLint config object and I don't know why...

You can take a look at this example where I've forked your yutak23:feature/migrate-to-v9 branch in my local, run the build script and added the dependency in my project.

You can see that the eslint-find-rules -p script is not listing the plugin rules for eslint-plugin-json

image I'm going to investigate another approach

@lotmek , i think,
By default, eslint-find-rules tries to find ESLint rules for files with the .js extension. So if you want to target .json like eslint-plugin-json, I think you need --ext .json as follows.

study@localhost:~/workspace/eslint-find-rules-test
$ npm run test:plugins

> [email protected] test:plugins
> eslint-find-rules --ext .json -p eslint.config.js


plugin rules

json/*                                   json/colon-expected                      json/comma-expected                      json/comma-or-close-backet-expected      json/comma-or-close-brace-expected
json/comment-not-permitted               json/duplicate-key                       json/enum-value-mismatch                 json/invalid-character                   json/invalid-escape-character
json/invalid-unicode                     json/json                                json/property-expected                   json/schema-resolve-error                json/trailing-comma
json/undefined                           json/unexpected-end-of-comment           json/unexpected-end-of-number            json/unexpected-end-of-string            json/unknown
json/value-expected

My project setup is as follows.
image

@ljharb
Copy link
Collaborator

ljharb commented Sep 30, 2024

We should also be automatically looking for files with .mjs or .cjs extensions, since those are supported in node.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be best to first add support for flat config, and then add support for eslint 9 in a separate PR.

.github/workflows/node.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
.mocharc.json Outdated Show resolved Hide resolved
.github/workflows/node.yml Outdated Show resolved Hide resolved
@yutak23 yutak23 requested a review from ljharb September 30, 2024 02:55
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure all files end in a trailing newline.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
@ljharb ljharb marked this pull request as draft September 30, 2024 09:23
@github-actions github-actions bot force-pushed the feature/migrate-to-v9 branch from 87315f9 to bb1d1a2 Compare October 1, 2024 01:12
@yutak23
Copy link
Author

yutak23 commented Oct 1, 2024

Please ensure all files end in a trailing newline.

i fixed this.

@yutak23 yutak23 requested a review from ljharb October 1, 2024 01:28
@yutak23 yutak23 marked this pull request as ready for review October 3, 2024 08:01
@yutak23 yutak23 force-pushed the feature/migrate-to-v9 branch from c974294 to 87315f9 Compare October 3, 2024 08:04
@github-actions github-actions bot force-pushed the feature/migrate-to-v9 branch from 87315f9 to bb1d1a2 Compare October 3, 2024 08:04
@yutak23 yutak23 requested a review from ljharb October 3, 2024 11:30
@ljharb ljharb marked this pull request as draft October 3, 2024 13:53
@Aluisio
Copy link

Aluisio commented Nov 5, 2024

Any news on the merge of this PR? There are many other libs that are waiting for this migration to also migrate to ESLint 9

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take this semver-major opportunity to set engines.node to '^18.18 || ^20.9 || ^22 || >= 23.1', and update CI to match.

test/lib/rule-finder.js Show resolved Hide resolved
test/lib/rule-finder.js Outdated Show resolved Hide resolved
test/lib/rule-finder.js Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@yutak23 yutak23 force-pushed the feature/migrate-to-v9 branch 2 times, most recently from 76bcbe1 to 06eda72 Compare November 6, 2024 04:27
@yutak23 yutak23 force-pushed the feature/migrate-to-v9 branch 2 times, most recently from 65d73e2 to 6b05df8 Compare November 6, 2024 06:51
@yutak23 yutak23 requested a review from ljharb November 6, 2024 07:04
.babelrc Show resolved Hide resolved
test/lib/rule-finder.js Outdated Show resolved Hide resolved
test/lib/rule-finder.js Outdated Show resolved Hide resolved
@yutak23 yutak23 requested a review from ljharb November 6, 2024 07:47
@ljharb ljharb force-pushed the feature/migrate-to-v9 branch from 8017235 to 4cc38cb Compare November 6, 2024 19:47
@ljharb ljharb marked this pull request as ready for review November 6, 2024 20:16
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good! I'll merge this after I've tested it on another project.

@niksy
Copy link

niksy commented Nov 16, 2024

FWIW, I’ve tried it in one of my projects where config was upgraded to ESLint 9 and flat config and it seems to be working as expected.

@robvdl robvdl mentioned this pull request Dec 7, 2024
14 tasks
@yutak23
Copy link
Author

yutak23 commented Dec 9, 2024

@ljharb,
Is it difficult to merge?
I would appreciate your confirmation.

@ljharb
Copy link
Collaborator

ljharb commented Dec 9, 2024

@yutak23

I'll merge this after I've tested it on another project.

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

Successfully merging this pull request may close these issues.

Support for ESLint 9.x?
5 participants