1305023 - `./mach eslint --setup` should install eslint into <topsrcdir>/node_modules/.bin, or symlink it - to make editor integration easy

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Banner (:standard8)]

Currently when running `mach eslint --setup` it installs into a directory under `/tools/lint/eslint/`.

Whilst that's ok for mach, that isn't good for all the editors out there that expect to find the binary under `<topsrcdir>/node_modules/.bin` (or they revert to the global installation).

This currently leads to hacks such as detailed in https://wiki.mozilla.org/DevTools/CodingStandards#Running_ESLint_in_SublimeText to specify a specific path, which then breaks when you are using the editor for different repositories.

We should either change the node_modules directory to be at the root, or provide a symlink.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

The problem is that different Firefox modules may have differing dependencies and using a top level node modules folder could easily cause conflicts.

At the same time, it would be much simpler for editors to integrate if we used a top level folder.

I think it makes most sense to use a top-level folder for linting and editor specific modules but use module specific folders otherwise.

Comment 2

[Mark Banner (:standard8)]

I took a bit of a look into this. It seems to make this work, we'd also need to move package.json into the root as well. I suspect that might make a few things a bit more awkward.

Atom has enough configuration options for making this work, and I think we could potentially create an atom config file at the top-level. Not sure we want to do that for every editor though.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

Review commit: https://reviewboard.mozilla.org/r/122708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122708/

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

I think we should go for something like this - it'll help with editor integration for developers.

I'd also like to follow this with seeing if we can do some form of automatic editor setup via ./mach bootstrap

Comment 5

[Mark Banner (:standard8)]

The try server run failed, but I think the method is still sane. I'll debug that later in the week.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

I'm all for this but I think we should get gps or someone to sign off on us adding this stuff at the top level.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

https://reviewboard.mozilla.org/r/122708/#review125138

Looks reasonable to me, definitely simplifies things for editors.

::: package.json:2
(Diff revision 1)
> +{
> +  "name": "Mozilla",

Should the name mention ESLint some way or another?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Dave Townsend [:mossop] from comment #6)
> Comment on attachment 8849955 [details]
> bug 1305023 - WIP Investigate moving node_modules to the top level for
> ESLint to improve editor integration.
> 
> I'm all for this but I think we should get gps or someone to sign off on us
> adding this stuff at the top level.

I guess the only thing actually being committed at the top level is the package.json, right?  The node_modules are ignored and local only.  But still, I agree it's good to clear the idea with someone like gps.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122708/diff/1-2/

Comment 10

[Mark Banner (:standard8)]

I fixed the issues (I'd got a couple of changes to the mozlint.yml file wrong), so switching the feedback to review request.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

https://reviewboard.mozilla.org/r/122708/#review128212

This makes me sad. I understand wanting to support default locations for things for convenience. But forcing your religion for a repository structure is just bad form. No wonder the JS community avoids monorepos.

I would strongly prefer to keep things out of the root directory because everything added there is potential for confusion and poor interaction with other tools. (I wouldn't be surprised if some random tool interprets this *package.json* as something not related to JS packaging and complains.)

But, if we need to do this to appease JS tools, we need to do this.

This change is small enough that it will be easy enough to revert if there are problems. So r+.

Also, I didn't realize we were downloading lots of files into a `node_modules` directory. Ideally we'd put those in `~/.mozbuild`, which is where we try to cache files used across multiple builds/checkouts. (We don't like writing files to the source directory that aren't under version control.)

Comment 12

[Mark Banner (:standard8)]

(In reply to Gregory Szorc [:gps] from comment #11)
> This makes me sad. I understand wanting to support default locations for
> things for convenience. But forcing your religion for a repository structure
> is just bad form. No wonder the JS community avoids monorepos.

I understand this point of view. If the editors had better configuration possibilities, we'd probably be in a better position, but like you say, the "norms" of the js community doesn't make this easy.

> I would strongly prefer to keep things out of the root directory because
> everything added there is potential for confusion and poor interaction with
> other tools. (I wouldn't be surprised if some random tool interprets this
> *package.json* as something not related to JS packaging and complains.)
> 
> But, if we need to do this to appease JS tools, we need to do this.

Ok, thank you for understanding.

> Also, I didn't realize we were downloading lots of files into a
> `node_modules` directory. Ideally we'd put those in `~/.mozbuild`, which is
> where we try to cache files used across multiple builds/checkouts. (We don't
> like writing files to the source directory that aren't under version
> control.)

Unfortunately, this is another case of editors expecting something in the root. Via npm shrinkwrap, we are limiting to specific versions of the modules. The builders also download a pre-tarred version. npm has its own cache so it is reasonable for developers, though obviously we still end up with the directory in the root.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8849955 [details]
Bug 1305023 - Move ESLint's package.json and node_modules to the top level to improve editor integration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122708/diff/2-3/

Comment 14

[Mark Banner (:standard8)]

The minor update I just posted was a switch to pushing via mercurial to work around git-cinnabar's lack of support for files that have changed after moving.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16bbacb61010
Move ESLint's package.json and node_modules to the top level to improve editor integration. r=gps

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16bbacb61010

Comment 17

[Kris Maglione [:kmag]]

This seems to have broken the commit hook: http://searchfox.org/mozilla-central/source/tools/mercurial/eslintvalidate.py

Comment 18

[Mark Banner (:standard8)]

(In reply to Kris Maglione [:kmag] from comment #17)
> This seems to have broken the commit hook:
> http://searchfox.org/mozilla-central/source/tools/mercurial/eslintvalidate.py

Thanks for pointing that out, I'm fixing it in bug 1354490.