Closed Bug 1305023 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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.
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.
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 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
Attachment #8849955 - Flags: feedback?(jryans)
Attachment #8849955 - Flags: feedback?(dtownsend)
Assignee: nobody → standard8
The try server run failed, but I think the method is still sane. I'll debug that later in the week.
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.
Attachment #8849955 - Flags: feedback?(gps)
Attachment #8849955 - Flags: feedback?(dtownsend)
Attachment #8849955 - Flags: feedback+
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?
(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.
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 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.)
Attachment #8849955 - Flags: review?(gps) → review+
(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.
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.
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/16bbacb61010
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1353385
Depends on: 1354460
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.