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

feat: add support for flat configs#7935

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

Merged
bradzacher merged 14 commits intomainfromflat-config-support-core-package
Feb 7, 2024

Conversation

bradzacher
Copy link
Member

@bradzacherbradzacher commentedNov 15, 2023•
edited
Loading

PR Checklist

Overview

This PR builds out support for Flat Configs for our project.

Unfortunately supporting both classic and flat configs from the plugin isa bit of a mess, so I took this opportunity to introduce something that we've always wanted - a single entrypoint package for linting users.

The beauty of this new package is that it means that linting users only need to install, update, and think about one package now - which is a big win.

This new package istypescript-eslint and it currently exports the following:

  • parser - an alias for@typescript-eslint/parser.
  • plugin - an alias for@typescript-eslint/eslint-plugin.
  • configs - flat config versions of the configs in@typescript-eslint/eslint-plugin.
  • config - a utility function to make it dead simple to use types in flat config files.

Consuming our tooling in a flat config will look something like this:

consttseslint=require('typescript-eslint');consteslintJs=require('@eslint/js');module.exports=tseslint.flatConfig(// if configuring manually...{plugins:{'@typescript-eslint':tseslint.plugin,},languageOptions:{parser:tseslint.parser,// parserOptions: { ... }},rules:{'@typescript-eslint/no-explicit-any':'error',},},// or using shared configseslintJs.configs.recommended,  ...tseslint.configs.eslintRecommended,  ...tseslint.configs.recommended,  ...tseslint.configs.stylistic,);

With this PR we are also dogfooding the package as I have migrated our config to a flat config. We're pretty light on shared configs so it's been pretty straight-forward and mostly "compat free". I have a few open TODOs in the config which are marked.

With this new tooling

TODO:

  • add documentation for the new package
  • add documentation for setting up our tooling with a flat config file
    • Might defer this to a follow-up PR to separate concerns and keep LOC down.
  • resolve TODOs in the flat config file
  • actually test the flat config file
  • update CI/IDE config/scripts to use the flat config
  • resolve discussion about what we should alias the plugin as.
    • Options are:
      • @typescript-eslint/ (the current approach in this PR)
      • typescript-eslint/
      • typescript/
      • ts-eslint/
      • ts/

tscpp, JoshuaKGoldberg, KristjanESPERANTO, supachaidev, and rob4226 reacted with thumbs up emojiauvred, shun-shobon, VictoriaSilver, talentlessguy, twhitbeck, joshuajaco, ild0tt0re, 4lph4-Ph4un, AdamVig, Unit2795, and 15 more reacted with hooray emojiJoshuaKGoldberg, shun-shobon, itsMapleLeaf, joshuajaco, cyrfer, ild0tt0re, roottool, 4lph4-Ph4un, Unit2795, schoero, and 6 more reacted with heart emojiild0tt0re, polyzen, 4lph4-Ph4un, nostalic, and shinGangan reacted with eyes emoji
@bradzacherbradzacher added the enhancementNew feature or request labelNov 15, 2023
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@bradzacher!

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.

@netlifyNetlify
Copy link

netlifybot commentedNov 15, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit8713862
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/65c2dd379f8dbe0008e92f8d
😎 Deploy Previewhttps://deploy-preview-7935--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		1 paths audited
Performance: 90 (🟢 up 2 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

rules: pluginBase.rules,
};

function flatConfig(...config: FlatConfig.ConfigArray): FlatConfig.ConfigArray {

Choose a reason for hiding this comment

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

Maybe justconfig? Cleaner to say & write, and after a year or so most new projects should only/primarily think about flat configs.

bradzacher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

the problem with calling itconfig is that you then have bothconfig andconfigs exported from the package.

It does make things a little confusing?

Choose a reason for hiding this comment

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

Hmm yeah never mind... 🤔 a tough naming situation.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Eh it works fine enough - I added it as is. People will deal with it it's not THAT bad.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commentedDec 27, 2023•
edited
Loading

Status update: this PR is blocked oneslint/eslint#17820. We're waiting for that to be resolved.eslint/eslint#17906 looks very promising and we'll try it out. Note that once we're unblocked, we'll have to limit flat config support to versions of ESLint with that fix in them.

Edit: the fix was merged but only into the v9 alphas. See#7935 (comment).

beaussan reacted with heart emoji

@JoshuaKGoldbergJoshuaKGoldberg added the blocked by external APIBlocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labelDec 27, 2023
@bradzacherbradzacherforce-pushed theflat-config-support-core-package branch from072c888 to4430fdeCompareDecember 30, 2023 03:12
@karlhorky

This comment was marked as off-topic.

@bradzacher
Copy link
MemberAuthor

Yeah we're trying to work with them to backport the fix we're waiting for to v8. Once we have it working on v8 then we can start to think about v9.

karlhorky reacted with heart emoji

@karlhorky

This comment was marked as off-topic.

@bradzacher
Copy link
MemberAuthor

bradzacher commentedJan 6, 2024•
edited
Loading

Filedeslint/eslint#17966 - we shall have to wait and see.

@woutermont

This comment was marked as off-topic.

@bradzacherbradzacher mentioned this pull requestJan 7, 2024
42 tasks
Comment on lines 26 to 28
function flatConfig(...config: FlatConfig.ConfigArray): FlatConfig.ConfigArray {
return config;
}
Copy link
MemberAuthor

@bradzacherbradzacherJan 13, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Question:
What do we think about making this utility enforce a canonical name for plugins?

For example imagine like:

// eslint.config.jsimport*astseslintfrom'@typescript-eslint/core';exportdefaulttseslint.config({'@typescript-eslint':tseslint.plugin,// ...},{},);

Then within the util we could rewrite all configs so that any references to the plugin use the defined namespace, and we rewrite it with pseudocode like:

for config in configs {  const pair = config.plugins.entries().find(plugin.value)  if pair && pair.key !== plugin.key {    pair.key = plugin.key    config.rules.keys().replace(`${pair.key}/`, `${plugin.key}/`)  }}

Pros: fixes the "multiple namespace problem"
Cons: rewrites user configs, includes some complexity.

WDYT?

Alternate forms:

  • more explicit object form likeconfig({ plugins: { ... }, configs: [ ... ] })
  • a util function form likeconfig.withPlugins({ ..plugins.. })(..configs[]) (whereconfig(..configs[]) acts without explicit enforcement)

Choose a reason for hiding this comment

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

I like the idea at first glance, but suspect it should be done in a follow-up issue so that the ESLint folks can provide input too.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Based on the discussions we've had (eslint/eslint#17766) - this is fully intended behaviour and it doesn't look like they intend to change that?

Choose a reason for hiding this comment

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

Oh I mean more on input for direction of the checking.

Plus splitting it out will make it easier to reference it when complaining about the name aliasing being allowed 😄

@woutermont
Copy link

@bradzacher, just want to make sure you saweslint/eslint#17820 (comment):

[Is the issue with null overrides] only a problem for the convenience config, or does it pop up elsewhere too?

To unblock the stalemate, might it be an idea to temporarily usefalse instead ofnull to communicate that disabling override?

@bradzacherbradzacherforce-pushed theflat-config-support-core-package branch from4430fde toa891a37CompareJanuary 24, 2024 04:39
@bradzacherbradzacherforce-pushed theflat-config-support-core-package branch fromad7e3c0 toee587e3CompareFebruary 2, 2024 07:28
@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedFeb 4, 2024•
edited
Loading

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 33 targets

Sent with 💌 fromNxCloud.

errorOnUnmatchedPattern: false,
- reportUnusedDisableDirectives: options.reportUnusedDisableDirectives || undefined,
+ // https://github.com/nrwl/nx/pull/21405
+ // reportUnusedDisableDirectives: options.reportUnusedDisableDirectives || undefined,
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@bradzacher
Copy link
MemberAuthor

@JoshuaKGoldberg - this is now truly ready for review (passing ci!!!)
Some initial docs are included as the package's docs.

karlhorky, JoshuaKGoldberg, schoero, and woutermont reacted with rocket emoji

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesFeb 6, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Whoohoo!! 🥳

This looks great to me. There are a few points that could be taken as followups (docs, tests, a bit of style) but nothing IMO that should block this being merged. Awesome job!

Choose a reason for hiding this comment

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

[Docs] This should be added tosidebar.base.js so it shows up & has a sidebar.

bradzacher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

UGH. We should automate this somehow.

);
} else {
flatCode.push(
`export default (_plugin: FlatConfig.Plugin, _parser: FlatConfig.Parser): FlatConfig.Config => (${flatConfigJson});`,

Choose a reason for hiding this comment

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

[Style] If they're unused, why bother including them?

Suggested change
`export default (_plugin: FlatConfig.Plugin, _parser: FlatConfig.Parser): FlatConfig.Config => (${flatConfigJson});`,
`export default (): FlatConfig.Config => (${flatConfigJson});`,

Here, and the unused import above?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

cos it sets things up to have a consistent API shape - which is nice in case we do change things in future.

I.e. it means we can blindly write

configs: {    all: allConfig(plugin, parser),    base: baseConfig(plugin, parser),    disableTypeChecked: disableTypeCheckedConfig(plugin, parser),    eslintRecommended: eslintRecommendedConfig(plugin, parser),    recommended: recommendedConfig(plugin, parser),    recommendedTypeChecked: recommendedTypeCheckedConfig(plugin, parser),    strict: strictConfig(plugin, parser),    strictTypeChecked: strictTypeCheckedConfig(plugin, parser),    stylistic: stylisticConfig(plugin, parser),    stylisticTypeChecked: stylisticTypeCheckedConfig(plugin, parser),}

rather than having to maintain which ones get args and which don't.

Choose a reason for hiding this comment

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

[Non-Actionable] Heh, good catch...

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

DAMN YOU AMERICANS SPELLING THINGS WRONG

Choose a reason for hiding this comment

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

IT'S NOT OUR FAULT WE DON'T HAVE AUSTRALIAN COFFEE

@bradzacherbradzacher merged commit8ef5f4b intomainFeb 7, 2024
@bradzacherbradzacher deleted the flat-config-support-core-package branchFebruary 7, 2024 02:52
aladdin-add added a commit to eslint/create-config that referenced this pull requestFeb 7, 2024
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsFeb 15, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

@auvredauvredauvred left review comments

@Josh-CenaJosh-CenaAwaiting requested review from Josh-Cena

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we mergeenhancementNew feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESLint Flat Config Support
5 participants
@bradzacher@JoshuaKGoldberg@karlhorky@woutermont@auvred
[8]ページ先頭
©2009-2025 Movatter.jp