Closed Bug 1365414 Opened 3 years ago Closed 3 years ago

eslint-plugin-mozilla does not play nice if installed globally

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bryce, Assigned: bryce)

Details

Attachments

(1 file)

I like to have my eslint install be global so that I can use it across projects. As such I also install my plugins globally. eslint-plugin-mozilla does not work if installed globally.

This appears to be due to rootDir's getter[1] assuming the module will be located within a central repo. However, if the file being linted is within the repo, but the plugin is not the following happens: eslint walks upwards and eventually enables the moz plugin after finding the root config, the moz plugin then walks up and eventually hits the FS root, linting then fails.

I've adjusted eslint-plugin-mozilla locally to also check from the current working directory. I've also considered walking from the target file being linted. This enables more diverse usage of the plugin. Are there any reasons why not to do this? If no, I can prepare a patch.

[1]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js#525
So the reason we need the root dir, is to try and find the "dynamic" files tools/lint/eslint/modules.json and browser/base/content/global-scripts.inc

I'm happy for a change to be made, as long as it still uses the "dynamic" files for m-c - the published version now falls back to local files, so if we hit the FS root, we could just declare it not found, and assume it isn't an m-c repo.
Comment on attachment 8868727 [details]
Bug 1365414 - Update eslint-plugin-mozilla to also search for MC root from CWD.

https://reviewboard.mozilla.org/r/140328/#review143986

r=Standard8 with the changes I've suggested.

::: commit-message-d8762:5
(Diff revision 1)
> +Bug 1365414 - Update eslint-plugin-mozilla to also search for MC root from CWD. r?standard8
> +
> +The eslint-plugin-mozilla currently searches for Mozilla Central root by
> +walking up from its installed dir. However, if the plugin is installed outside
> +of central, such as gloablly, it will not find the root. This changeset

s/gloablly/globally/

::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:545
(Diff revision 1)
> +
> +      let possibleRoot = searchUpForIgnore(path.dirname(module.filename));
> +      if (!possibleRoot) {
> +        possibleRoot = searchUpForIgnore(path.resolve());
> +      }
> +      if(!possibleRoot) {

nit: missing space (as picked up by eslint).

::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:545
(Diff revision 1)
> +
> +      let possibleRoot = searchUpForIgnore(path.dirname(module.filename));
> +      if (!possibleRoot) {
> +        possibleRoot = searchUpForIgnore(path.resolve());
> +      }
> +      if(!possibleRoot) {

The if could potentially just be inside the other if, to avoid the double-check when it is found from the module location.
Attachment #8868727 - Flags: review?(standard8) → review+
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2be1833d89d2
Update eslint-plugin-mozilla to also search for MC root from CWD. r=standard8
https://hg.mozilla.org/mozilla-central/rev/2be1833d89d2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Firefox Build System
Assignee: nobody → bvandyk
You need to log in before you can comment on or make changes to this bug.