1347709 - Unexpected require when using eslint-plugin-mozilla@0.2.29

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Peter deHaan [:pdehaan]]

Ref: https://github.com/mozilla-services/pageshot/issues/2365#issuecomment-286882614

In Page Shot, we're trying to use **eslint-plugin-mozilla@0.2.29** and are getting errors because eslint-plugin-mozilla is trying to require a non-existent file (in eslint-plugin-mozilla/lib/environments/places-overlay.js):


```
var root = helpers.getRootDir(module.filename);

var tmp = path.join(root, "tools", "lint", "eslint", "modules.json");

console.log(tmp);
var modules = require(tmp);
```

Locally, this logs the following value: 

/Users/pdehaan/dev/github/mozilla-services/pageshot/tools/lint/eslint/modules.json


I'm guessing that's why this is crashing hard, because we don't have a "tools/lint/eslint/modules.json" file in tbe Page Shot repo tree (but it looks like it _is_ a path in the m-c repo: http://searchfox.org/mozilla-central/source/tools/lint/eslint/modules.json).

Comment 1

[Peter deHaan [:pdehaan]]

Found this dynamic require in two other places:

1. https://hg.mozilla.org/mozilla-central/file/tip/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js#l187
2. https://hg.mozilla.org/mozilla-central/file/tip/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js#l221

Comment 2

[Ed Lee :Mardak]

I also ran into this for activity-stream, and a quick workaround at least for local development is to symlink to tools from the top level of your repository:

https://github.com/mozilla/activity-stream/pull/2304

ln -s ../mozilla-central/tools/

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1347709 - Add a .npmignore file to stop publishing unnecessary files for eslint-plugin-mozilla.

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

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1347709 - Add a prepublish script to save the current globals for the published version of eslint-plugin-mozilla, and use that when not in mozilla-central.

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

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1347709 - Allow modules.json to be loaded from a local version for out-of-tree uses of eslint-plugin-mozilla.

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

Comment 6

[Mark Banner (:standard8)]

I've tried the patches against the one-off-system-addon & screenshots repo, and the code seems to work & rules pass.

Once this lands I'll get a new npm version published and start work on adding it to those repos properly.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8858136 [details]
Bug 1347709 - Add a .npmignore file to stop publishing unnecessary files for eslint-plugin-mozilla.

https://reviewboard.mozilla.org/r/130096/#review132992

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8858137 [details]
Bug 1347709 - Add a prepublish script to save the current globals for the published version of eslint-plugin-mozilla, and use that when not in mozilla-central.

https://reviewboard.mozilla.org/r/130098/#review132998

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8858138 [details]
Bug 1347709 - Allow modules.json to be loaded from a local version for out-of-tree uses of eslint-plugin-mozilla.

https://reviewboard.mozilla.org/r/130100/#review133008

::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:47
(Diff revision 1)
>  
>  module.exports = {
> +  get modulesGlobalData() {
> +    if (!gModules) {
> +      if (this.isMozillaCentralBased()) {
> +        console.log("m-c based");

Remove the logging please

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8858138 [details]
Bug 1347709 - Allow modules.json to be loaded from a local version for out-of-tree uses of eslint-plugin-mozilla.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af51323bf014
Add a .npmignore file to stop publishing unnecessary files for eslint-plugin-mozilla. r=mossop
https://hg.mozilla.org/integration/autoland/rev/2153975f1124
Add a prepublish script to save the current globals for the published version of eslint-plugin-mozilla, and use that when not in mozilla-central. r=mossop
https://hg.mozilla.org/integration/autoland/rev/c873d3edeae6
Allow modules.json to be loaded from a local version for out-of-tree uses of eslint-plugin-mozilla. r=mossop

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ce69b6e1773e
and Bug 1356569 - Bump eslint plugin version to resolve conflict. r=instruction-by-florian a=bustage-fix

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/af51323bf014
https://hg.mozilla.org/mozilla-central/rev/2153975f1124
https://hg.mozilla.org/mozilla-central/rev/c873d3edeae6

Comment 14

[Ed Lee :Mardak]

What's the expected behavior with these changes? Repositories must add a `modules.json` in its root directory with keys of module filenames and values of string array of exported symbols?

Comment 15

[Ed Lee :Mardak]

I'm running into some issues after `npm install mozilla-central/tools/lint/eslint/eslint-plugin-mozilla`

Looks like it's trying to read a "./modules.json" relative to eslint-plugin-mozilla/lib/helpers.js:
    at Object.get modulesGlobalData [as modulesGlobalData] (activity-stream/node_modules/eslint-plugin-mozilla/lib/helpers.js:49:20)

If I symlink to a modules.json that I put at the top level, it then

Error: Cannot find module './environments/saved-globals.json'
    at Object.getSavedEnvironmentItems (activity-stream/node_modules/eslint-plugin-mozilla/lib/helpers.js:593:12)

Maybe I'll just wait for Standard8 to fix another repository, and I'll just copy. :)

Comment 16

[Dave Townsend [:mossop]]

You need to wait for Standard8 to publish the module to npm then install that.

Comment 17

[Mark Banner (:standard8)]

Ed: There's a prepublish script that gets run, however you can run that manually before importing the module locally.

I also have just published it to npm, so you can pick up the new version (0.2.39) there (and there's a minor bug in the prepublish script that I'll publish a fix for tomorrow).

Comment 18

[Mark Banner (:standard8)]

Actually, for some reason 0.2.39 wasn't published right. 0.2.40 works.