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 eslint 9 #38

Merged
merged 2 commits into from
Dec 27, 2024
Merged

feat: support eslint 9 #38

merged 2 commits into from
Dec 27, 2024

Conversation

Smrtnyk
Copy link
Contributor

@Smrtnyk Smrtnyk commented Dec 20, 2024

Also increased major version

I tested this in my repo https://github.com/Smrtnyk/Firetable where I am using eslint 9 and flat config, and it works fine with these changes

I ran the tests:all command it also passes all the tests.

lib/index.js Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Smrtnyk Smrtnyk force-pushed the support-eslint-9 branch 2 times, most recently from 18d1697 to cfadc82 Compare December 21, 2024 17:16
Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

The rest LGTM, thanks! :)

lib/index.js Show resolved Hide resolved
Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

Code LGTM.

We may need some docs changes... and we also should figure out how v9+ handle the config presets (given that other breakage you were discussing).

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 23, 2024

Code LGTM.

We may need some docs changes... and we also should figure out how v9+ handle the config presets (given that other breakage you were discussing).

I can help you with that.
Do you want it to be part of this PR or separate?
I believe this PR is enough to get the people going and unblock them from eslint 9 upgrade (such as myself at the company I work for), and then I can contribute another PR with flat config preset.
I believe that flat config preset could require some restructuring of the plugin object, so it might involve more code changes.
Either way I want to help, so let me know what you prefer :)

@getify
Copy link
Owner

getify commented Dec 23, 2024

removing the preset would be a "breaking change", which sort of defeats the intent here of "fixing" the plugin to keep working without such forking/breakage.

so I would like to at least understand better what other changes are necessary to preserve presets before wrapping this up.

it might well belong in a separate PR, but I think we need that to land before a new "release"... so I just want to have a better sense of the "scope" of such work before we finalize this PR.

also, if this plugin is now used/activated/configured differently in v9+, I want to make sure the docs have a mention of that difference.

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 23, 2024

removing the preset would be a "breaking change", which sort of defeats the intent here of "fixing" the plugin to keep working without such forking/breakage.

so I would like to at least understand better what other changes are necessary to preserve presets before wrapping this up.

it might well belong in a separate PR, but I think we need that to land before a new "release"... so I just want to have a better sense of the "scope" of such work before we finalize this PR.

also, if this plugin is now used/activated/configured differently in v9+, I want to make sure the docs have a mention of that difference.

I think we can leave your preset as it is
and add a new one maybe called getify-says/flat.
I had a bit of a look at how eslint promise plugin did this
They have entries for both legacy and flat configs
https://github.com/eslint-community/eslint-plugin-promise/blob/main/index.js#L51
I tried something like this but there were some exceptions in the eslint server and cli linter, which made me starting to restructure the current plugin object.
I think exceptions came due to some self referencing objects.

How I used this plugin with eslint 9 when testing was like this:
when I removed the plugins array which you aasked about, I continued using getify-says preset, but I added proper arrows plugin manually myself to the plugins object in my flat config.
After I reverted the plugins change in getify-says preset, I had to remove it from my flat config and manually declare the plugin + rules.

I could push a separate commit that adds the preset and required refactoring (if needed) and then we can discuss it
whether it is acceptable solution for you or not

Regarding the docs, I am not a native english speaker but I can give it a try

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 23, 2024

I have pushed a separate second commit, that adds a flat config preset
you can see how it is used in here Smrtnyk/Firetable@be20260#diff-a32a0887ed9d1d707bbb3b845b7df7fd40e673c47e7b60a3ebd896b68d3b8839R25

If that is fine for you I can then add some docs for this

@Smrtnyk Smrtnyk force-pushed the support-eslint-9 branch 2 times, most recently from 9d8952c to dd30aa3 Compare December 23, 2024 15:02
@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 23, 2024

btw I noticed that there is no CI build on PR branches.
I see that there is a travis config file but I am not able to see the builds.
I would feel more comfortable if there was a build and test github action running when I push to the branch.
I am building and running tests locally, but still.
If you are open for a small PR to setup a github action to run build and test I can contribute

@getify
Copy link
Owner

getify commented Dec 23, 2024

yes, happy to look at any and all contributions, especially helpful ideas like the CI stuff. :)

@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 23, 2024

yes, happy to look at any and all contributions, especially helpful ideas like the CI stuff. :)

added a simple workflow in #39
tested it locally using act and docker, seems like it works fine

 Also increased major version
@Smrtnyk Smrtnyk force-pushed the support-eslint-9 branch 2 times, most recently from b32e8eb to 40e6d1b Compare December 24, 2024 10:22
@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 24, 2024

@getify In second commit I have updated the readme and added a flat config entry for the getify-says preset
I have tested it in my repo https://github.com/Smrtnyk/Firetable/blob/flat-config-arrow-functions-eslint-plugin/eslint.config.js#L25 and it works nice

Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

This is looking good, appreciate it. Just a few minor tweaks.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
 Add flat config preset
@Smrtnyk
Copy link
Contributor Author

Smrtnyk commented Dec 26, 2024

This is looking good, appreciate it. Just a few minor tweaks.

tnx for the feedback Kyle
I have force pushed the requested changes to the last commit

Copy link
Owner

@getify getify left a comment

Choose a reason for hiding this comment

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

LGTM!

@getify getify merged commit 42b9e02 into getify:master Dec 27, 2024
2 checks passed
@getify
Copy link
Owner

getify commented Dec 27, 2024

NOTE: I'll be delaying publishing this updated version on npm for a bit, just to give us time to get some feedback from folks if they try it.

@Smrtnyk Smrtnyk deleted the support-eslint-9 branch December 27, 2024 07:08
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.

2 participants