Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: switch to ESLint flat config#6836

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
nzakas wants to merge10 commits intotypescript-eslint:mainfromnzakas:flat-config

Conversation

nzakas
Copy link
Contributor

PR Checklist

:)

Overview

In testing the new flat config, I thought a good way to test typescript-eslint would be to convert the project itself to use flat config. I've got most of the way there, but I'm afraid my knowledge of how to configure Nx is pretty basic and so I can't quite get the linting to run. Happy to continue iterating on this if someone can point me in the right direction.

JoshuaKGoldberg, armano2, Rec0iL99, acommodari, michaelfaith, britalmeida, azat-io, thenbe, tkrotoff, acidoxee, and kachkaev reacted with rocket emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@nzakas!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint.

@netlify
Copy link

netlifybot commentedApr 3, 2023
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit93e3d43
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/64888cb98eb353000763ac30
😎 Deploy Previewhttps://deploy-preview-6836--typescript-eslint.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 yourNetlify site settings.

@armano2armano2 changed the titlechore: Switch to ESLint flat configchore: switch to ESLint flat configApr 3, 2023
@bradzacher
Copy link
Member

thanks heaps for doing this! Even went and fixed up the nx config for us!
will have a proper look at it shortly.

quick question from a glance:FlatCompat what are the steps the ecosystem needs to take in order for us to migrate away from the compat class? Is the big ticket item defining flat config compatible configurations from our plugin?

britalmeida reacted with heart emoji

@nzakas
Copy link
ContributorAuthor

quick question from a glance: FlatCompat what are the steps the ecosystem needs to take in order for us to migrate away from the compat class? Is the big ticket item defining flat config compatible configurations from our plugin?

Yes. So basically my thinking was first, get the repo's config files swapped over toeslint.config.js and make sure everything works withFlatCompat as a sanity check. Then, the next step would be to create flat config versions of the other configs you include the plugin. I'm also happy to help with that once we can get the repo linted properly usingeslint.config.js.

JoshuaKGoldberg and bradzacher reacted with thumbs up emoji

@nx-cloud
Copy link

nx-cloudbot commentedApr 5, 2023
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit93e3d43. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


🟥 Failed Commands
nx run-many --target=lint --parallel
✅ Successfully ran 24 targets

Sent with 💌 fromNxCloud.

@nzakas
Copy link
ContributorAuthor

So this is where I need help. I just don't know enough about Nx to understand what the problem is with the lint task.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commentedApr 5, 2023
edited
Loading

I'll defer to@JamesHenry as the Nx expert in the org, but Isuspect that Nx doesn't yet recognizeeslint.config.js. You may need to have the Nx config specify the path explicitly:https://nx.dev/packages/linter/executors/eslint#eslintconfig. (found vianrwl/nx#8982)

(I'm holding time on my calendar to try this out tomorrow morning, if nobody else has gotten it fixed by then) ✔️ James replied

@JamesHenry
Copy link
Member

Hi@nzakas, thanks again for this!

@JoshuaKGoldberg is exactly right, for now in all project.json files with a "lint" target you will need to add theeslintConfig property.

Now that you have confirmed that flat config is feature complete we will add automatic detection for this in an upcoming Nx version but for now you'll need to configure it, e.g for the website project:

image

There seem to be some remaining issues, however, and I wanted to hand it back to you before I spend too long spinning my wheels on stuff I'm not familiar with.

2 issues that seem like they have easy solutions:

  1. The references torequire('@typescript-eslint') need to be updated because that isn't a module that can be resolved (I think you used the value from the old config directly which was relying on ESLint's magical resolution of the eslint-plugin package under that namespace). The requires should be:

image

  1. eslintrc.extends is being given arrays here but seems to be expecting multiple strings so it throws

At least one remaining issue that I can't easily understand from the schema validation error message:

image

I wonder if anything can be done to build on top of ajv and improve that error message?

JoshuaKGoldberg reacted with thumbs up emoji

@nzakas
Copy link
ContributorAuthor

@JoshuaKGoldberg@JamesHenry great, I'll make those changes and see how far I can get.

@nzakas
Copy link
ContributorAuthor

Okay, so the root level problem I'm dealing with right now is that because Nx uses--config with the specified configuration file, that automatically is treated as eslintrc instead of flat config. In order to force ESLint use flat config with--config, we need to set the environment variableESLINT_USE_FLAT_CONFIG=true. I've tried a few different things to set an environment variable but it seems like however Nx is using ESLint isn't respecting it. Any ideas?

@JamesHenry
Copy link
Member

@nzakas Nx doesn't use the CLI--config it uses theESLint class API. If I add a console.log tonode_modules/@nrwl/linter/src/executors/eslint/lint.impl.js I can see the environment variable being respected, is that definitely enough? Or does Nx need to conditionally use the new class in its executor implementation?

@JamesHenry
Copy link
Member

The exact class usage is here:node_modules/@nrwl/linter/src/executors/eslint/utility/eslint-utils.js

@nzakas
Copy link
ContributorAuthor

@JamesHenry ah! Yes, that makes a big difference. It's the CLI that recognizes the environment variable and then switches betweenESLint andFlatESLint classes, so the Nx implementation would have to do the same (assuming it wants to support flat config).

@JamesHenryJamesHenry added the blocked by external APIBlocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labelApr 6, 2023
@JamesHenry
Copy link
Member

JamesHenry commentedApr 6, 2023
edited
Loading

@nzakas cool, I'll submit a PR to Nx next week when I'm back from vacation to allow it to:

A) automatically detecteslint.config.js files (once it is the default format we will also switch Nx to generating/updating them like it does today with.eslintrc.json files)

B) Conditionally change the implementation class depending on the config used/the presence of the environment variable

Until then I've mark this one as blocked, I'll circle back as soon as there is an Nx version for us to apply here

@nzakas
Copy link
ContributorAuthor

Awesome, thanks so much. Enjoy your vacation!

@JamesHenry
Copy link
Member

Just a follow up on this, now that we (Nx) are a larger company with more resources to work on product, we have a more robust planning cycle. I have added supporting flat config as an item for our next iteration which starts in a couple of weeks. I should hopefully be working on it personally.

I will update this PR once there is a version to try it out with.

Thanks again@nzakas!

acommodari reacted with eyes emoji

@JamesHenryJamesHenry marked this pull request as draftApril 20, 2023 10:18
@nzakas
Copy link
ContributorAuthor

Sure thing. I'll stay tuned in to this PR.

@nzakasnzakas mentioned this pull requestApr 20, 2023
69 tasks
@JamesHenryJamesHenry removed the blocked by external APIBlocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labelJun 9, 2023
@JamesHenry
Copy link
Member

Hi@nzakas I'm sorry it took me a while to loop back to this. I added the support for Flat Config to Nx in the last cycle and have updated your branch here to contain those changes.

However, when I run the linting for a particular project, it seems that the root config is currently invalid based on the new structure?

image

@nzakas
Copy link
ContributorAuthor

Sorry about that. I think the config file was so long that I just missed that section. Indeed, there are no overrides in flat config, so I just flattened that section out and pushed the new config file. (I'm on my way out so didn't have time to verify it 100% works but at least it's the correct format.)

@JamesHenry
Copy link
Member

@nzakas it did fail to validate again just FYI

@nzakas
Copy link
ContributorAuthor

TypeError: Key "plugins": Cannot redefine plugin "eslint-plugin".

This means more than one config is specifyingeslint-plugin and they're not using the same plugin object. In this case, it's likely that one of the eslintrc configs is usingeslint-plugin-eslint-plugin, which is being pulled in byrequire, andeslint.config.js is pulling it in usingimport.

For now I've just commented out the offending lines in the configs. For some reason, I can't get the lint in the website package to run correctly locally, so relying on CI to help figure that part out.

@GitMurf
Copy link

Hi team! Nice work so far. I have a couple projects coming up that I am proposing and they are requiring some clarity on the transition to the new flat configs. Is this the PR to follow for the transition of typescript-eslint to using the new flat config file formateslint.config.js? If not, is there a better place to "follow" / comment? Thanks!

jviall and thenbe reacted with thumbs up emoji

@bradzacherbradzacher added the repo maintenancethings to do with maintenance of the repo, and not with code/docs labelJul 17, 2023
@JoshuaKGoldberg
Copy link
Member

Just a heads up, we've also got#7273 in-progress. Might be relevant here.

michaelfaith and GitMurf reacted with thumbs up emoji

@JoshuaKGoldbergJoshuaKGoldberg mentioned this pull requestSep 26, 2023
3 tasks
@JoshuaKGoldberg
Copy link
Member

@nzakas now thateslint/eslint#17640 is merged, are there any changes you want to make here?

@nzakas
Copy link
ContributorAuthor

That PR doesn't really apply here. This PR's purpose was to switch the repo to use flat config. Updating the plugin for flat config would be separate.

@JoshuaKGoldberg
Copy link
Member

Got it - what I really mean is, is there anything on your end you plan on doing to this PR? Or is it ready for us to take over?

@JamesHenry
Copy link
Member

JamesHenry commentedOct 18, 2023
edited
Loading

I think a win for everybody here would be if we abandon this PR and instead run the Nx automated migration for migrating from .eslintrc to flat config:

  • it saves Nicholas's time
  • it tests out the migration on a real world repo and helps us find any remaining rough edges

WDYT?

@nzakas
Copy link
ContributorAuthor

Automation FTW! My feelings are not hurt if a robot can do this better than me. 😄

JoshuaKGoldberg, JamesHenry, Lutra-Fs, and jenewland1999 reacted with laugh emoji

@karlhorky
Copy link

@JamesHenry I guess you mean this Nx automated migration to flat config?

Looks pretty cool 👀 didn't know there was a tool to automate this! I guess it could work for other configs elsewhere too... likeoureslint-config-upleveled shared config 🤔 Added to our issue tracking the flat config:upleveled/eslint-config-upleveled#160

@JamesHenry
Copy link
Member

JamesHenry commentedOct 20, 2023
edited
Loading

@karlhorky to be clear the automation is not for plugin authors to convert their plugin implementations to flat config, it is for users to convert their setups.

This PR relates to typescript-eslint’s own usage of eslint

@karlhorky
Copy link

Yeah, maybe it still applies to shared configs like the one I shared above? (it's not a plugin, although we also have one of those)

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsOct 28, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

Assignees

@JamesHenryJamesHenry

Labels
repo maintenancethings to do with maintenance of the repo, and not with code/docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@nzakas@bradzacher@JoshuaKGoldberg@JamesHenry@GitMurf@karlhorky

[8]ページ先頭

©2009-2025 Movatter.jp