1426240 - Linting problem: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/extensionSupport.jsm:68:24 | 'fixIterator' is not defined.

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Jorg K (CEST = GMT+2)]

Strange, since it is defined in iteratorUtils.jsm which is imported. What am I missing?

Comment 1

[:aceman]

Can the linter see inside .jsms what objects are imported? But if this would be a problem, almost all .js files importing jsms would be flagged in this way. So how is extensionSupport.jsm special?
walkExtensionPrefs() is called async from the addonmanager. May it be that the imported objects are out of scope at that time? But we know locally that the code works and does not complain about the missing fixIterator.

Comment 2

[:aceman]

Actually case 1 could be it, we just do not see the problem in other files because all others are excluded from linting. But common/* is a new folder and is missing from .eslintignore yet. Of course I'd like we actually enable linting of all our files one day.
So we may keep this bug to determine what needs to be done to solve this particular type of problem.

Comment 3

[:aceman]

But then also IOUtils should have been reported...

Comment 4

[:aceman]

I have got the ES linter green with https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f4095b3b5fb0ea9728c8232f9ac38ee86296bf8f but I still do not understand why it does not complain about IOUtils too. Is it a difference that IOUtils.js exports a single object (named the same), but iteratorUtils.jsm does three?

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think you should add the new directory to eslintignore. If you want to enable a new directory, you will have to define a number of linting rules and agree on style, as I did in calendar/.eslintrc.js. Lightning disables this specific rule because I thought the globals importing stuff was messy.

Comment 6

[:aceman]

Thanks, yes I think the linter does not see imported objects so we could turn off this rule globally for now. See bug 1426459 for the mess it outputs.

I would leave this directory enabled to be linted as it contains a single file and we could try to keep it without warnings as a testbed. We could try to go with the defaults and see what happens, e.g. there are no other warnings in this file, so our current style seems fine for the linter.

Comment 7

[Tom Prince [:tomprince]]

Having everything that is being imported named specifically certainly makes it easier to see where stuff comes from. I know most python projects I've worked on tend to frown on 'from module import *' for exactly that reason.

Comment 8

[:aceman]

Attachment: 1426240.patch

Turning off no-undef seems to help much in general.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8938268 [details] [diff] [review]
1426240.patch

I would not want to turn this on globally, such decisions should be per-subdirectory. You are welcome to add this to common/.eslintrc.js though.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

As a note, m-c uses https://dxr.mozilla.org/comm-central/source/mozilla/tools/lint/eslint/modules.json in combination with https://firefox-source-docs.mozilla.org/tools/lint/linters/eslint-plugin-mozilla.html#import-globals but we cannot extend this file.

Comment 11

[:aceman]

Attachment: 1426240.patch v2

Ok, moved the setting under /common .

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8938674 [details] [diff] [review]
1426240.patch v2

Review of attachment 8938674 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc

::: common/.eslintrc.js
@@ +4,5 @@
> +  // We would like the same base rules as provided by
> +  // mozilla/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
> +  "extends": [
> +    "plugin:mozilla/recommended"
> +  ],

This is already inherited by the toplevel .eslintrc.js, all you need here is the rules section.

Comment 13

[:aceman]

Attachment: 1426240.patch v2.1

Thanks.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/63658572abf9
turn off linter warning of undefined identifiers due to too many false positives in /common. r=philipp

Comment 15

[Intermittent Failures Robot]

1 failures in 590 pushes (0.002 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* comm-central: 1

Platform breakdown:
* lint: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1426240&startday=2017-12-18&endday=2017-12-24&tree=all