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: add support for flat configs #7935

Merged
merged 14 commits into from Feb 7, 2024
Merged

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Nov 15, 2023

PR Checklist

Overview

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

Unfortunately supporting both classic and flat configs from the plugin is a 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 is typescript-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:

const tseslint = require('typescript-eslint');
const eslintJs = 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 configs
  eslintJs.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/

@bradzacher bradzacher added the enhancement New feature or request label Nov 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 on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Nov 15, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 8713862
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65c2dd379f8dbe0008e92f8d
😎 Deploy Preview https://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 your Netlify site configuration.

rules: pluginBase.rules,
};

function flatConfig(...config: FlatConfig.ConfigArray): FlatConfig.ConfigArray {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem with calling it config is that you then have both config and configs exported from the package.

It does make things a little confusing?

Copy link
Member

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
Member Author

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.

eslint.config.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Dec 27, 2023

Status update: this PR is blocked on eslint/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).

@JoshuaKGoldberg JoshuaKGoldberg added the blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API label Dec 27, 2023
@bradzacher bradzacher force-pushed the flat-config-support-core-package branch from 072c888 to 4430fde Compare December 30, 2023 03:12
@karlhorky

This comment was marked as off-topic.

@bradzacher
Copy link
Member Author

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

This comment was marked as off-topic.

@bradzacher
Copy link
Member Author

bradzacher commented Jan 6, 2024

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

@woutermont

This comment was marked as off-topic.

@bradzacher bradzacher mentioned this pull request Jan 7, 2024
42 tasks
Comment on lines 26 to 28
function flatConfig(...config: FlatConfig.ConfigArray): FlatConfig.ConfigArray {
return config;
}
Copy link
Member Author

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.js
import * as tseslint from '@typescript-eslint/core';

export default tseslint.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 like config({ plugins: { ... }, configs: [ ... ] })
  • a util function form like config.withPlugins({ ..plugins.. })(..configs[]) (where config(..configs[]) acts without explicit enforcement)

Copy link
Member

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
Member Author

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?

Copy link
Member

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 saw eslint/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 use false instead of null to communicate that disabling override?

@bradzacher bradzacher force-pushed the flat-config-support-core-package branch from ad7e3c0 to ee587e3 Compare February 2, 2024 07:28
Copy link

nx-cloud bot commented Feb 4, 2024

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

Choose a reason for hiding this comment

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

@bradzacher
Copy link
Member Author

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

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Feb 6, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg 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!

Copy link
Member

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 to sidebar.base.js so it shows up & has a sidebar.

Copy link
Member Author

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.

docs/packages/TypeScript_ESLint.mdx Show resolved Hide resolved
docs/packages/TypeScript_ESLint.mdx Outdated Show resolved Hide resolved
);
} else {
flatCode.push(
`export default (_plugin: FlatConfig.Plugin, _parser: FlatConfig.Parser): FlatConfig.Config => (${flatConfigJson});`,
Copy link
Member

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
Member Author

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.

Copy link
Member

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
Member Author

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

Copy link
Member

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

@bradzacher bradzacher merged commit 8ef5f4b into main Feb 7, 2024
63 of 65 checks passed
@bradzacher bradzacher deleted the flat-config-support-core-package branch February 7, 2024 02:52
aladdin-add added a commit to eslint/create-config that referenced this pull request Feb 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESLint Flat Config Support
5 participants