1619842 - eslint integration with vscode broken in browser/components/newtab

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P2)

Description

[Kate Hudson :k88hudson]

As of last week(?) it looks like vscode's eslint plugin is having trouble finding the right packages when trying to lint code inside browser/component/newtab. I see a bunch of errors like this:

Definition for rule 'getter-return' was not found
Definition for rule 'no-global-assign' was not found
Definition for rule 'no-return-await' was not found

Some observations, although I don't know what the cause / fix is for this:

• deleting browser/components/newtab/node_modules fixes the issue, so probably a problem with the eslint plugin not adding the right folders to the node PATH
• mach lint is unaffected

Comment 1

[Kate Hudson :k88hudson]

Adding this to my vscode workspace settings temporarily fixes issue, although I think we need a solution that doesn't involve doing this manually:

{
  "eslint.nodePath": "./node_modules"
}

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

It's too bad that eslint doesn't have an option to just replace the head of the command (so that we could start our command with something like mach lint -l eslint). We'd have to prototype to see if that was really a good solution. I wonder if the extension author would accept a patch for that...

Comment 3

[Mark Banner (:standard8)]

Given that removing browser/components/newtab/node_modules fixes the issue, I think it'd be worth figuring out what can be done with saaslint (which has an older version of ESLint as a dependency). I've had issues with ESLint in the past where there's been two different versions in tree, and the wrong version can be picked up at times.

You can probably confirm this by just removing browser/components/newtab/node_modules/eslint (and maybe eslint-scope).

We might be able to add "root": true to newtab's .eslintrc.js, and install ESLint into newtab again. However, I think it could still cause problems depending on what ESLint does. It'd also mean that newtab's config would be separate to the rest of m-c, which I'm not really keen on - I'd like to start working to harmonise the configs and get a clearer picture of what is different and why.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

This is apparently fixed in the master branch of sasslint, but sasslint hasn't been released from master in almost a year: https://github.com/sasstools/sass-lint/issues/1288

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Newtab could consider switching to stylelint: https://stylelint.io/

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

There's a lot to like about stylelint, but it has many more dependencies than sasslint. On the other hand, there's high security value in a package that is actually actively maintained.

Since the fix is on sasslint master, the simplest thing we could do is just create our own release based on sasslint-master and depend on that. We could do that by forking sasslint-master, but since we're not really changing the code, we don't even necessarily have to do that.

I'd suggest that we start with the sasslint master , and file a separate bug to evaluate stylelint. Standard8, any opinion on the best way to do this forking or non-forking?

Comment 7

[Mark Banner (:standard8)]

Sorry, no opinions. If you publish anything on npm, you might get stuck maintaining it though...

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

My thinking was not to publish it on npm. I was sort of thinking of keeping a local copy in the tree e.g. in tools/lint/sasslint. Thoughts?

Comment 9

[Shane Hughes [:aminomancer]]

Something useful I found: https://dev.to/stories_of_ren/switching-from-sass-lint-to-stylelint-5f8c

Comment 10

[Shane Hughes [:aminomancer]]

Attachment: Bug 1619842 - Migrate from sass-lint to stylelint. r=#omc-reviewers

Fix a bug where VS Code's eslint integration does not work for the
newtab project because of a conflict caused by sass-lint's eslint
dependency. Switching over to stylelint eliminates that dependency and
generally modernizes our SCSS linting. stylelint doesn't have a 1 to 1
replacement for every sass-lint rule, so a few rules have been changed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84a1894bdd4d
Migrate from sass-lint to stylelint. r=omc-reviewers,thecount,emcminn

Comment 12

[Shane Hughes [:aminomancer]]

Attachment: Bug 1619842 - Remove longhand rule from newtab stylelint config. r=CosminS

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b496bf4aa91a
Remove longhand rule from newtab stylelint config. r=CosminS

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/84a1894bdd4d
https://hg.mozilla.org/mozilla-central/rev/b496bf4aa91a