Skip to content

Update ESLint & Prettier to latest - #463

Merged
eemeli merged 1 commit into
mozilla:masterfrom
eemeli:update-deps
Nov 3, 2022
Merged

Update ESLint & Prettier to latest#463
eemeli merged 1 commit into
mozilla:masterfrom
eemeli:update-deps

Conversation

@eemeli

Copy link
Copy Markdown
Member

This should make it easier to work with the project, as atm it appears to require using npm i --legacy-peer-deps to get running.

ESLint rules that are covered by Prettier are dropped, and a "prettier" config is added to the package.json to account for the default changes in its v1 -> v2 transition.

@eemeli
eemeli requested a review from ochameauOctober 6, 2022 07:54

@hybristhybrist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Skimmed the formatting changes, RSLGTM.

Comment thread.eslintrc.js Outdated
Comment thread.eslintrc.js
@@ -239,9 +196,6 @@ module.exports = {
// Disallow tabs.
"no-tabs": "error",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would tabs already be replaced by prettier?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

There are places like comments where Prettier doesn't touch tabs, while this rule would complain about them.

Comment thread.eslintrc.js Outdated
eemeli added a commit to eemeli/source-map that referenced this pull request Oct 21, 2022
eemeli added a commit to eemeli/source-map that referenced this pull request Oct 21, 2022
@eemeli

Copy link
Copy Markdown
MemberAuthor

Rebased on master to trigger CI jobs.

Comment threadpackage.json

@ochameauochameau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for looking into this!

Note that when running npm run prettier locally I see modifications against .waiting.html. It might relate to your very latest change.
Shouldn't we have a check against prettier on Github actions?
It looks like we only run eslint.
Would it be easy to run prettier and fail in case of modification?

This should make it easier to work with the project, as atm it appears to require using npm i --legacy-peer-deps to get running.

This is for my own culture (feel free to ignore), but for me it works without --legacy-peer-deps.

$ npm i
npm WARN eslint-plugin-prettier@3.4.1 requires a peer of eslint@>=5.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.13 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.13: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
removed 157 packages and audited 779 packages in 4.887s
62 packages are looking for funding
run `npm fund` for details
found 98 vulnerabilities (17 moderate, 59 high, 22 critical)
run `npm audit fix` to fix them, or `npm audit` for details
$ npm run lint
=> WORKS
$ npm run prettier
=> ALSO WORKS

As if the warning during npm i is harmless?
(I'm on npm 6.14.17 and node v14.20.1)

Otherwise I confirm that this prettier error during npm i disappear with your patch.

ESLint rules that are covered by Prettier are dropped, and a "prettier" config is added to the package.json to account for the default changes in its v1 -> v2 transition.

I tried to find what rules prettier was applying without much success. Were you able to find such list online?
Again, this isn't super important as your patch looks good as-is.

I'd be more interested in trying to align slightly more with mozilla-central linting.
Regarding prettier rules, I see the following failures:

lib/wasm.js
115:12 error Insert `,` prettier/prettier (eslint)
116:10 error Insert `,` prettier/prettier (eslint)
129:10 error Insert `,` prettier/prettier (eslint)
source-map.js
6:29 error Delete `⏎·` prettier/prettier (eslint)
8:28 error Delete `⏎·` prettier/prettier (eslint)
and many others...

Otherwise beyond prettier, there is a few rules on mozilla-central that fails:

Use the global form of 'use strict'. strict (eslint)
Parameter 'aArgs' uses Hungarian Notation, consider using 'args' instead. mozilla/no-aArgs (eslint)
use .includes instead of .indexOf mozilla/use-includes-instead-of-indexOf (eslint)
Expected { after 'if' condition. curly (eslint)
error Prefer boolean length check mozilla/prefer-boolean-length-check (eslint)

We can also look into that in followups and not necessarily align all rules.

@eemeli

Copy link
Copy Markdown
MemberAuthor

Note that when running npm run prettier locally I see modifications against .waiting.html. It might relate to your very latest change.

The .waiting.html file looks like it's been accidentally included in the repo in #335, and should be dropped. We should do that separately from this PR.

Shouldn't we have a check against prettier on Github actions? It looks like we only run eslint. Would it be easy to run prettier and fail in case of modification?

Yes, we could/should also check the Prettier styling in CI, and adding that is pretty easy. I left it out here as that'd be an additional change to the nominal version update that's done here.

This is for my own culture (feel free to ignore), but for me it works without --legacy-peer-deps.

Good point! I'll drop that here as well.

As if the warning during npm i is harmless? (I'm on npm 6.14.17 and node v14.20.1)

It looks like that goes away if you wipe out the node_modules directory first.

I tried to find what rules prettier was applying without much success. Were you able to find such list online? Again, this isn't super important as your patch looks good as-is.

That's handled by eslint-config-prettier, which disables these rules.

I'd be more interested in trying to align slightly more with mozilla-central linting.
[...]
Otherwise beyond prettier, there is a few rules on mozilla-central that fails:
[...]
We can also look into that in followups and not necessarily align all rules.

Good point. I'm going to minimise the diff of this PR by not applying the updated Prettier formatting, and file another PR applying the m-c Prettier config and a stripped-down version of the m-c ESLint config here.

@eemeli
eemeli merged commit aa8555f into mozilla:masterNov 3, 2022
@eemeli
eemeli deleted the update-deps branch November 3, 2022 20:33
Sign up for freeto 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.

3 participants

@eemeli@ochameau@hybrist