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

ESLint: use the --rulesdir option for internal rules. #3849

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

IvanGoncharov
Copy link
Member

@IvanGoncharov IvanGoncharov commented Feb 16, 2023

Motivation: We need to have a way to have internal ESLint rules. ESLint allows you to import rules in two ways:

  1. As an npm package, it is problematic for internal rules since we don't want to publish it on NPM. So in the past, we used NPM-specific workarounds to treat a directory as a package.
  2. --rulesdir CLI option, the problem with it is deprecated.

ESLint team is planning to deprecate the current config format and switch to: https://eslint.org/docs/latest/use/configure/configuration-files-new
This format forces you to import plugins manually, and it entirely resolves our problem with internal rules. But it requires that all plugins that we use to support this format.

I propose to temporarily switch to --rulesdir and start working on changing to the new format since the entire config format will be deprecated anyway. This switch will unblock support for node@19 in CI and local development and unblock #3848.

Motivation: We need to have a way to have internal ESLint rules.
ESLint allows you to import rules in two ways:
1. As an npm package, it is problematic for internal rules since we don't want to publish it on NPM. So in the past, we used NPM-specific workarounds to treat a directory as a package.
2. `--rulesdir` CLI option, the problem with it is deprecated.

ESLint team is planning to deprecate the current config format and switch to:
https://eslint.org/docs/latest/use/configure/configuration-files-new
This format forces you to import plugins manually, and it entirely resolves our problem with internal rules. But it requires that all plugins that we use to support this format.

I propose to temporarily switch to `--rulesdir` and start working on changing to the new format since the entire config format will be deprecated anyway.
This switch will unblock support for node@19 in CI and local development and unblock graphql#3848.
@netlify
Copy link

netlify bot commented Feb 16, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit f6d84c1
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63eff2c644b6990008f9783f
😎 Deploy Preview https://deploy-preview-3849--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

Hi @IvanGoncharov, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR
Copy link
Contributor

@IvanGoncharov that sounds great to me.

We can also update the package.json and README files within resources/eslint-internal-rules

We still need the package.json to mark the file as CJS, but we don't really need much in there, we can even take out the package name.

The README should be updated to reflect that the need for the package.json is just to mark the files as cjs (which won't be necessary with the new config format!) and updated as well in that you no longer need to run npm install to pick up changes.

Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

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

Looks good, we could update the package.json and README as I commented separately, but that could be a separate PR.

@IvanGoncharov
Copy link
Member Author

Thanks, @yaacovCR.
I implemented your suggestions before merging.

@IvanGoncharov IvanGoncharov merged commit 3efdc54 into graphql:main Feb 17, 2023
@IvanGoncharov IvanGoncharov deleted the pr_branch4 branch February 17, 2023 21:38
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Feb 17, 2023
Context: As disscussed in graphql#3849 ESLint team is planning to deprecate the current config format and switch to:
https://eslint.org/docs/latest/use/configure/configuration-files-new
But it requires that all plugins that we use to support this format.

In my PoC attempt to use this new format biggest change is comming from converting YAML file into JS, so I decided to extract it into separate commit.
Note: beyound JS convertion I did a purely stylistic cleanup of `TODO` comments, beyound that this commit is pure convertion of YAML into JS.
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Feb 17, 2023
Context: As disscussed in graphql#3849 ESLint team is planning to deprecate the current config format and switch to:
https://eslint.org/docs/latest/use/configure/configuration-files-new
But it requires that all plugins that we use to support this format.

In my PoC attempt to use this new format biggest change is comming from converting YAML file into JS, so I decided to extract it into separate commit.
Note: beyound JS convertion I did a purely stylistic cleanup of `TODO` comments, beyound that this commit is pure convertion of YAML into JS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants