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 flat config system #592

Closed
wants to merge 16 commits into from

Conversation

filiptammergard
Copy link
Contributor

@filiptammergard filiptammergard commented Oct 18, 2023

Resolves #591.

Wanted to open this for review to get some early feedback. This seems to work fine, if I create an eslint.config.js in the root with this content:

const prettier = require("./flat");

module.exports = [
  prettier.configs.recommended,
];

I get ESLint errors accordingly.

The idea here is that the flat config file could be imported this way in other projects:

const prettier = require("eslint-plugin-prettier/flat");

module.exports = [
  prettier.configs.recommended,
];

The reason not to change the main export is that the eslintrc format needs to have continued support. Eslintrc will be deprecated in v9 of ESLint and removed in v10, so to introduce a minimal amount of breaking changes, the flat config support can be purely additive and when ESLint v10 is released, the flat config could be made the main export in a new major version of this plugin.

Thoughts? If you think it's a good approach, I can continue adding documentation add changeset etc.

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2023

🦋 Changeset detected

Latest commit: 9c45c5d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-prettier Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@filiptammergard
Copy link
Contributor Author

Any early input in this one @JounQin?

@JounQin
Copy link
Member

JounQin commented Oct 24, 2023

Sorry for forgetting this, I'll review recently ASAP.

@pumano
Copy link

pumano commented Oct 24, 2023

Would love to use it as it will be available! @filiptammergard thank you!

recommended.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
recommended.js Outdated Show resolved Hide resolved
@filiptammergard
Copy link
Contributor Author

Some new docs on how to migrate to flat config format just came out: https://eslint.org/docs/latest/extend/plugin-migration-flat-config. Did some updates to align with those recommendations.

recommended.js Outdated Show resolved Hide resolved
@pumano
Copy link

pumano commented Oct 25, 2023

@filiptammergard don't stop in your idea to support it)

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Per https://eslint.org/blog/2023/10/flat-config-rollout-plans/

You should also make sure that your rules aren’t using context.parserOptions and context.parserPath. Instead, you should be using context.languageOptions and context.languageOptions.parser, which also work when ESLint is run in eslintrc mode. See our previous post for more information.

Looking at https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/, it seems we need to update our usage of the now-deprecated:

  • context.getSourceCode
  • context.getFilename
  • context.getPhysicalFilename

We will need to identify when the replacements for these functions were added, and update our minimum supported version of eslint accordingly.

Any change to the minimum supported version will be a breaking change (Which is fine, but we'll need to update the changeset).

recommended.js Outdated Show resolved Hide resolved
@filiptammergard
Copy link
Contributor Author

filiptammergard commented Oct 26, 2023

Per https://eslint.org/blog/2023/10/flat-config-rollout-plans/

You should also make sure that your rules aren’t using context.parserOptions and context.parserPath. Instead, you should be using context.languageOptions and context.languageOptions.parser, which also work when ESLint is run in eslintrc mode. See our previous post for more information.

Looking at https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/, it seems we need to update our usage of the now-deprecated:

  • context.getSourceCode
  • context.getFilename
  • context.getPhysicalFilename

We will need to identify when the replacements for these functions were added, and update our minimum supported version of eslint accordingly.

Any change to the minimum supported version will be a breaking change (Which is fine, but we'll need to update the changeset).

Good input @BPScott!

Looks like the methods you're mentioning are becoming properties (added in v8.40.0): https://eslint.org/blog/2023/09/preparing-custom-rules-eslint-v9/#context-methods-becoming-properties

The methods will be removed in ESLint v10 (not v9), so there's no rush here. The blog post also suggests supporting both versions like this:

const sourceCode = context.sourceCode ?? context.getSourceCode();

If the property is undefined (as per before ESLint v8.40.0), the method is used instead, and no breaking change needs to be considered.

However, I think those changes can be considered separately from supporting flat config format, so I made a separate PR for it: #594

@filiptammergard
Copy link
Contributor Author

@JounQin @BPScott Updated now to avoid mutations and to align with the recommendation to always provide meta information: https://eslint.org/docs/latest/extend/plugin-migration-flat-config#adding-plugin-meta-information I needed to bump @types/eslint to get support for flat config types.

Also renamed from recommended to flat since that makes it more clear that this in a flat config export.

package.json Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
flat.js Outdated Show resolved Hide resolved
flat.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.changeset/kind-apples-itch.md Outdated Show resolved Hide resolved
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Thanks for fighting through this!

https://eslint.org/docs/latest/extend/plugin-migration-flat-config#backwards-compatibility makes it seem like it would be totally fine to have both a regular and flat config defined within the configs key of eslint-plugin-prettier.js instead of needing a whole new file for this.

It seems like it should be possible to add a new "recommended-flat" key to the configs object and define the config there and use it like so, instead of needing to add a new file to the repo:

// in some consuming apps eslint.config.mjs file that is authored in the flat config style

import eslintPluginPrettier from 'eslint-plugin-prettier';

export default [
  eslintPluginPrettier.configs['recommended-flat'],
  {/* the app's own config goes here */}
]

The linked blogpost suggested having a flat config called "BLAH" and the legacy equivalent is named named "BLAH-legacy". That seems like a nice idea eventually, but I like the idea of of avoiding a backwards compatibility break now by adding a new config that is flat-compatible and leaving the current one as-is. We can swap the names around in some future major version.

Could you please add the new flat compatible config to the configs key in eslint-plugin-prettier.js with the name"recommended-flat", instead of adding a new entrypoint to the package.


We'll also need to update the README to talk about how to use the flat config, but hold off on that till we are happy with the the implementation and how people would consume it.

@filiptammergard
Copy link
Contributor Author

filiptammergard commented Oct 29, 2023

@BPScott I think both ways work, and for example eslint-plugin-react went the other way: https://github.com/jsx-eslint/eslint-plugin-react#configuring-shared-settings

But since ESLint recommends doing as you said, I think it might be a good idea to lean on that. The downside with having them in the same file is that I can't reference the plugin from the config like this:

const eslintPluginPrettier = {
  configs: {
    recommended: {
      extends: ['prettier'],
      plugins: ['prettier'],
      rules: {
        'prettier/prettier': 'error',
        'arrow-body-style': 'off',
        'prefer-arrow-callback': 'off',
      },
    },
    'recommended-flat': {
      plugins: {
        prettier: eslintPluginPrettier, // Error "Block-scoped variable 'eslintPluginPrettier' used before its declaration.ts(2448)"
      },
      rules: {
        'prettier/prettier': 'error',
        'arrow-body-style': 'off',
        'prefer-arrow-callback': 'off',
      },
    },
  },
  rules: {
    prettier: {/* ... */},
  },
};

That makes eslintPluginPrettier reference itself and that gives a circular dependency. So we have to either mutate it with Object.assign, or we create a new variable as I did now.

Thoughts on this?

@filiptammergard
Copy link
Contributor Author

@BPScott I saw you pushed a commit for using Object.assign instead of creating a new variable. Any other changes requested, or should I get going updating the docs accordingly?

@pumano
Copy link

pumano commented Nov 3, 2023

@JounQin could you please review it? Very important feature

'prefer-arrow-callback': 'off',
},
},
['recommended-flat']: {
Copy link
Member

Choose a reason for hiding this comment

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

Should we use camelCase here?

Copy link

Choose a reason for hiding this comment

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

looks like config names should be as rules with dashes also eslint have config naming convention for eslint plugins with dashes too

Copy link
Member

@JounQin JounQin Nov 3, 2023

Choose a reason for hiding this comment

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

The previous rules are for legacy configs, for flat configs, we're using js object reference instead, so camelCase is preferred IMO. And I think flatRecommended makes more sense than recommendedFlat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

recommended-flat is all based on direction from the ESLint team:

Export both eslintrc and flat configs. The configs key is only validated when a config is used, so you can provide both formats of configs in the configs key. We recommend that you append older format configs with -legacy to make it clear that these configs will not be supported in the future. For example, if your primary config is called recommended and is in flat config format, then you can also have a config named recommended-legacy that is the eslintrc config format.

https://eslint.org/docs/latest/extend/plugin-migration-flat-config#backwards-compatibility

They're recommending appending -legacy for legacy configs (to make it recommended-legacy for example), which means -flat is the equivalent. Should we not go for that?

Copy link
Member

Choose a reason for hiding this comment

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

That strategy is an unnecessary breaking change IMO, and it can be made in next major version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why the suggestion is to add recommended-flat now (non-breaking) and swap them around in next major.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is to add flatRecommended config key instead of recommended-flat.

eslint-plugin-prettier.js Outdated Show resolved Hide resolved
eslint-plugin-prettier.js Outdated Show resolved Hide resolved
@BPScott
Copy link
Member

BPScott commented Nov 3, 2023

Ah sorry, I thought I added a comment after pushing up that change at the weekend.

I've been thinking about this for the last few days, and now I've had a chance to play with what the eslint suggested approach looks like, I actually kinda hate it. Having to do object reassignment in eslint-plugin-prettier feels clunky, and the naming clashes of recommended vs recommended-flat/flat-recommended is awkward.

This PR's original approach of mimicking eslint-plugin-react and creating a new recommended.js file, and getting people to import from eslint-plugin-prettier/recommended feels a bunch cleaner. We should feed this back to the eslint team at some point.

@JounQin I think my previous request of "lets follow eslint recommendations" was a bad idea (well, good for learning what it looks like, bad for actually using), and instead am preferring having the "recommended.js" file that was proposed at the start of this PR. Now you've saw both approaches, do you have on preference on which one feels better?

@JounQin
Copy link
Member

JounQin commented Nov 3, 2023

In my view I'd like to have a flat.js exporting a flat plugin, usage:

const prettier = require("eslint-plugin-prettier/flat");

module.exports = [
  {
    // usage 1, use plugin manually
    plugins: {
      prettier,
    },
  },
  // usage 2, enable rules automatically
  prettier.configs.recommended,
];

@BPScott
Copy link
Member

BPScott commented Nov 3, 2023

I think "flat plugin" is a bit of a misnomer. Reading https://eslint.org/docs/latest/extend/plugin-migration-flat-config, I get the impression that if we add the meta block to the existing plugin then it will work with flat or legacy configs, with the exception of the configs object. At that point it's only the "configs" that we need to deal with. I'm pretty sure that for usage one, people can already do the following (once we add the meta block), there's no need for a dedicated file:

const prettier = require("eslint-plugin-prettier");

module.exports = [
  {
    // usage 1, use plugin manually
    plugins: {
      prettier,
    },
  },
];

And then usage 2 can be

const prettierRecommended = require("eslint-plugin-prettier/recommended");

module.exports = [
  {
    ...prettierRecommended
  },
];

@filiptammergard
Copy link
Contributor Author

And same for this one: #603

@filiptammergard
Copy link
Contributor Author

I dropped the commit where @types/eslint was bumped from this PR now as it is already taken care of in #602.

```js
const prettier = require('eslint-plugin-prettier');

module.exports = [prettier.configs['recommended-flat']];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module.exports = [prettier.configs['recommended-flat']];
module.exports = [prettier.config.flatRecommended];

I still recommend this config name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But even better with a separate flat.js file, right?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have such preference, a clear document is enough IMO.

@filiptammergard
Copy link
Contributor Author

Waiting for input by @BPScott on what direction to take before I proceed. I'll vote for using a separate file flat.js.

@filiptammergard
Copy link
Contributor Author

Do you have any input @BPScott?

@JounQin
Copy link
Member

JounQin commented Nov 29, 2023

I think the deadline could be this weekend, otherwise I'll help to refactor and release in my view.

@BPScott
Copy link
Member

BPScott commented Nov 30, 2023

Waiting for input by @BPScott on what direction to take before I proceed. I'll vote for using a separate file flat.js.

Aah sorry I totally missed this and the updates from the eslint discord. I was planning to come back to this PR last week but needing a break got to me.


@filiptammergard thank you for pushing on with this, and asking for opinions from the eslint discord. It's great to know that they're not pushing hard on the approach suggested in https://eslint.org/docs/latest/extend/plugin-migration-flat-config.

Doing a little audit of other popular plugins:

With three popular packages diverging from the recommendations I don't feel bad about diverging. We have a single rule so we won't ever need multiple configs (ignoring the legacy vs flat distinction) so I think placing that in a configs folder is overkill. The root of the repo is fine and good.

I think I still prefer creating a new separate file and usage being const prettierRecommended = require('eslint-plugin-prettier/recommended');. I think that's a little cleaner than const prettierRecommended = require('eslint-plugin-prettier').configs['flat/recommended'].

I would prefer that new file be named recommended.js instead of flat.js. Consider fast forwarding to two years time when ESLint v10 is released and the legacy config format is dead - having a file called flat.js feels strange in that world - I'd expect that the filename should be the name of the config - recommended feels like it makes more sense to me.


My request is annoyingly "back to what we had originally before I opened my mouth":

  • A recommended.js in the root of the repo
  • Add a recommended.d.ts with types
  • Add those two files to the package.json's files array
  • We'll need to update the README to describe usage when using flat config

@BPScott
Copy link
Member

BPScott commented Dec 4, 2023

I've made a the above changes in #616. Notably in contains a rewrite of the readme file's configuration section.

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.

Support ESLint flat config
5 participants