1395426 - Eslint globals reference doesn't work for bug 1349689 (cyclic dependencies with import-globals-from)

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[Ricky Chien [:rickychien] (inactive)]

I'm seeing eslint throwing no-undef errors at [1], it's really weird and these globals (DownloadUtils, LoadContextInfo) should be detected and included by our Mozilla eslint tool.

This is more likely an issue in our custom eslint configuration.

In order to not block bug 1349689, bug 1349689 is going to add following workaround

/* globals DownloadUtils, LoadContextInfo */

on the top of browser/components/preferences/in-content/privacy.js.

Note that we should have a follow-up patch to get rid of above workaround once eslint issue gets fixed.

stardard8, would you like to take at look of this? Thanks

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c8995dd9ce7&selectedJob=127034426

Comment 1

[Mark Banner (:standard8)]

Found the issue - we have a cache when loading globals from other files to save having to re-parse each time.

Due to our lovely cyclic interdependencies between js files (I've a feeling preferences/ is one of the worst), we have a stop-gap to avoid circular dependencies causing eslint to go in a loop - but that isn't working quite right in this case:

   If you trigger something that loads globals for a.js -> b.js -> a.js, then the second call to get globals for 'a.js' returns early (empty), so 'b.js' caches its own globals without the ones for 'a.js'. Then when you hit b.js later, the globals are wrong.

In the case of this bug we saw that running:

./mach eslint browser/components/preferences

would trigger the bug, but

./mach eslint browser/components/preferences/in-content/

would not trigger it. That's all due to the order in which the files happen to get loaded.

We either need to get the cache to not save it in the intermediate case (which might affect perf), or rewrite the cache mechanism (maybe store just the globals for the file itself, and which sub-files it references).

Given the changes needed I'll unlikely get to this before 57 branches, but given that we have the workaround via the 'global' comment, I think that is ok.

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1395426 - When determining globals for ESLint, handle circular dependencies correctly. r?#frontend-codestyle-reviewers!

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1395426 - Inline AssignmentExpression and CallExpression handlers in eslint-plugin-mozilla globals.mjs. r?#frontend-codestyle-reviewers!

This avoids calling the more generic Expression statement all the time, and simplifies processing of the node objects.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a4955e751219
https://hg.mozilla.org/integration/autoland/rev/fc47fe4e363f
When determining globals for ESLint, handle circular dependencies correctly. r=frontend-codestyle-reviewers,Gijs
https://github.com/mozilla-firefox/firefox/commit/1222b6e0050e
https://hg.mozilla.org/integration/autoland/rev/eefdcb69e220
Inline AssignmentExpression and CallExpression handlers in eslint-plugin-mozilla globals.mjs. r=frontend-codestyle-reviewers,Gijs

Comment 6

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/fc47fe4e363f
https://hg.mozilla.org/mozilla-central/rev/eefdcb69e220