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

Switch to new eslint config format #991

Closed
wants to merge 4 commits into from

Conversation

Cruikshanks
Copy link
Member

DEFRA/water-abstraction-team#115

In Add linting using eslint with standard as the base we switched from using standard directly for linting to using ESlint.

This is all part of the work we are doing to start codifying our conventions rather than writing them up for no one to read! 😁

We then went to make a config change but saw that we were using a deprecated core rule and instead needed to use @stylistic/eslint-plugin-js for max-len rule. So, we fixed that! 💪

But then we clocked ESLint was telling us our .eslintrc.js config file format is deprecated! 😩 We instead should be using the eslint.config.js flat file config.

So, before we start making changes to our config this change gets us using the latest supported format.

DEFRA/water-abstraction-team#115

In [Add linting using eslint with standard as the base](#948) we switched from using [standard](https://www.npmjs.com/package/standard) directly for linting to using [ESlint](https://eslint.org/).

This is all part of the work we are doing to start codifying our conventions rather than writing them up for no one to read! 😁

We then went to make a config change but saw that we were using a deprecated core rule and instead needed to [use @stylistic/eslint-plugin-js for max-len rule](#989). So, we fixed that! 💪

But then we clocked ESLint was telling us our `.eslintrc.js` [config file format is deprecated](https://eslint.org/docs/latest/use/configure/configuration-files-deprecated)! 😩 We instead should be using the `eslint.config.js` [flat file config](https://eslint.org/docs/latest/use/configure/configuration-files).

So, before we start making changes to our config this change gets us using the latest supported format.
The new flat file format no longer supports `extends:`. Instead, in flat config files

> predefined configs are imported from separate modules into flat config files.

This means you need to `import` your config as you would any other package then specify what you've imported at the top of your config. For example this

```javascript
module.exports = {
  extends: 'standard',
  // ...
```

becomes this

```javascript
const standard = require('eslint-config-standard')

module.exports = [
  standard,
  {
    //...
```
Note - This will error when run
@Cruikshanks Cruikshanks added do not merge Used for spikes and experiments housekeeping Refactoring, tidying up or other work which supports the project labels May 8, 2024
@Cruikshanks Cruikshanks self-assigned this May 8, 2024
@Cruikshanks
Copy link
Member Author

This change is currently blocked. Admittedly we are new to this. But from our reading to switch away from the now deprecated config file format we cannot just have ESLint call out to standard. We have to follow what the docs say about Predefined and Shareable Configs.

This means we have to use eslint-config-standard. The problem is when we do we get this error.

Oops! Something went wrong! :(

ESLint: 8.57.0

A config object is using the "parserOptions" key, which is not supported in flat config system.

Flat config uses "languageOptions.parserOptions" to specify parser options.

Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#configuring-language-options

And it appears we are not the only ones. Now the PR references ESLint v9 and we appear to be on v8. But the problem seems to be the same. There are some workarounds suggested and we can keep an eye on them. But more worrying is the last issue added; A standard that lacks maintenance is no longer a standard.

No one has responded to it or closed the issue.

So, we don't want to switch file formats yet because doing so forces a dependency on eslint-config-standard. And we don't want that until we see some traction on these issues.

@Cruikshanks
Copy link
Member Author

This has been superseded by #1476

Cruikshanks pushed a commit that referenced this pull request Nov 22, 2024
DEFRA/water-abstraction-team#115

Now, we are a team of seven, with seven different opinions on the 'right' way to write the code! Because of our size, we are splitting into two teams to focus on various features simultaneously. But we'll still be working with the one code base.

We'd already moved from just using [StandardJS](https://standardjs.com/) to lint our code to using **StandardJS** via **ESLint** (we kept the rules but not the tool) because there are too many cases where **StandardJS** has no ruling, but we wanted one.

However, each time these rules don't provide a style convention, the team must stop, discuss, debate, and finally decide how something will be done. We want something else to take the decision away from us!

Step forward [Prettier](https://prettier.io/). **Prettier** is an opinionated code formatter that focuses only on the style of the code.

> Prettier enforces a consistent code style (i.e. code formatting that won’t affect the AST) across your entire codebase because it disregards the original styling by parsing it away and re-printing the parsed AST with its own rules that take the maximum line length into account, wrapping code when necessary.

So, any rules or conventions we have that would affect the [Abstract Syntax Tree (AST)](https://www.nearform.com/insights/what-is-an-abstract-syntax-tree/) would still be controlled by **ESlint**. These are the things as a team it is worth spending time debating and agreeing.

For the rest, we intend to let **Prettier** take over. It is widely used across the JavScript community and is popularly advocated for teams that become large or dispersed like ours.

Our approach to the adoption was first to remove our existing **ESlint** config, add **Prettier**, and then let it update all the files.

Then, having checked through as a team there were no 'red-line' changes, commit them. We then add **ESlint** back in and only re-apply the non-stylistic rules.

Our research found that JSDoc is still better managed through **ESlint** (**Prettier** does nothing with it), so we also added our original config for that.

Another fundamental change is that we are no longer bringing in **StandardJS** as an **ESlint** extension. This means we can move to the latest **ESlint** v9 and away from the [deprecated config file format](https://eslint.org/docs/latest/use/configure/configuration-files-deprecated) to the new [flat file config](https://eslint.org/docs/latest/use/configure/configuration-files).

Previously, we'd been blocked because extensions are not supported in ESLint v9. **StandardJS** does provide [eslint-config-standard](https://github.com/standard/eslint-config-standard), but along with the core project, it does not appear to be actively maintained, is still using the deprecated **ESLint** style rules and has an error so cannot be used.

> See [Switch to new eslint config format](#991) for more details on why this was blocking us

We'd resigned ourselves to manually copying and updating the rules from their plugin when we revisited an old issue when first switching to **ESLint** with **StandardJS**. [Support for Eslint v9 Flat Config format](standard/eslint-config-standard#411) was one of the blockers that meant to have both **ESLint** and **StandardJS** play ball, we were stuck on ESLint v8.

We spotted that there had been some [recent activity](standard/eslint-config-standard#411 (comment)), all of which referenced an alternative called [neostandard](https://github.com/neostandard/neostandard).

And oh gosh! All our dreams were answered.

- Built for ESlint to avoid the need for separate IDE tooling
- Built for the latest ESLint (v9), so flat-file config is supported
- Just like we did, any style rules have been updated to use @stylistic/eslint-plugin
- An explicit desire to work with current practices. So, built for use with ESLint only, banning or requiring `;` is now an option, and disabling style rules for those opting to use **prettier** is possible

For context, maintenance on **StandardJS** and related packages like **eslint-config-standard** has been stalled for some time. **neostandard** references the issue as being a [block in governance and direction of travel](standard/standard#1948 (comment)). I've not been through every message, but it appears the maintainers are split between those who remained committed to StandardJS's 'one-tool one-way' approach and those looking to move to where most folks are: **ESLint**.

Even our own @johnwatson484 [has gotten involved!](standard/standard#1948 (comment)).

We're betting this isn't going to be resolved any time soon, so to avoid us having to maintain standard rules in our ESLint config manually, the final fundamental change to highlight is we're now using ESLint + neostandard to manage coding rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Used for spikes and experiments housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant