1766099 - Can we drop modules.json dependency of importScript ?

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task)

Description

[Tooru Fujisawa [:arai]]

modules.json is used for the following 3 purpose:

1. ChromeUtils.import, importing into this or globalThis
2. Cu.import (Components.utils.import), importing into this or globalThis
3. importScripts, except for the file in the same directory

The first 2 are going to be solved by bug 1758481 and bug 1765167,
but importScripts doesn't have a way to explicitly import variables, and still depends on modules.json to get the list of exported globals.

As pointed out in bug 1548308, modules.json tends to become out of date,
and indeed, it's already very inconsistent with the actual symbols exported by each module,
that means the linting doesn't guarantee the correct result.

Then, given the other 2 cases are going away, and the number of importScripts that requires modules.json isn't too much, we could revisit the design here.

Possible solutions are:

• (a) Instead of listing the exported symbols for each module, provide a map between the path (e.g. resource:// URI) to the in-tree path, and load globals from the file, in the same way as the filename-only case
• (b) Require import-globals-from above each importScripts
• (c) Require global above each importScripts

(a) is similar to the current situation, but more robust against the change to the imported script.

(b) is already used in some case, and it works well if the imported script is from the same component

(c) is almost same as what ChromeUtils.import requires. each importScripts consumer needs to list symbols they want to import.

Comment 1

[Tooru Fujisawa [:arai]]

Tried the combination of (a) and (b), and that looks reasonably small change

Comment 2

[Mark Banner (:standard8)]

I'm fine with a combination of (a) and (b). I wouldn't really want to do (c) as that may require updating/checking every time we change the exports of a script.

At one time, :Mossop was discussing changing our resource/chrome URIs to be based on the source tree directories rather than "random" locations - which would make this easier to look up files, but I don't see that happening in the short term.

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #1)

Tried the combination of (a) and (b), and that looks reasonably small change

Sorry, my comment was wrong.

what I've tried was:

• the combination of (a) and (c) (map file + global)
• the combination of (b) and (c) (import-globals-from + global)

both seems to be reasonably small.

(c) was required mostly for the case that the current globals.js file cannot figure out the exported symbols, such as require.js and worker helper.js, that indirectly defines the global variable.

I'll check if that can be replaced with other directive (maybe global and exported in those files?)

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Mark Banner (:standard8) from comment #2)

I wouldn't really want to do (c) as that may require updating/checking every time we change the exports of a script.

Just to make sure, I'm thinking about listing symbols that's imported and actually used in the file, instead of listing all symbols exported by the file.
So, in the situation that the change requires update to the global comment, it means the JS code itself should also be updated.
But I agree that it's better if that can automatically be done by linter.
will try addressing the issue about require.js etc above.

(In reply to Mark Banner (:standard8) from comment #2)

At one time, :Mossop was discussing changing our resource/chrome URIs to be based on the source tree directories rather than "random" locations - which would make this easier to look up files, but I don't see that happening in the short term.

That sounds really good in the long term solution :)
Figuring out the relation between the resource:// URI and the real path is always tedious ;)

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: option (b) : import-globals-from

Here's WIP patch for option (b).
(actually 11 patches locally, for each directory https://treeherder.mozilla.org/jobs?repo=try&revision=21e48e3eacfcbe2a9a1e6f06bfd8b856cdc7e1d3
).

Comment 6

[Mark Banner (:standard8)]

Yeah, I think that's reasonable given our current infrastructure.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 1: Explicitly specify import or imported symbol for importScripts in browser/components/newtab/. r=Standard8

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 2: Explicitly specify import or imported symbol for importScripts in devtools/. r=Standard8

Depends on D144671

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 3: Explicitly specify import or imported symbol for importScripts in dom/. r=Standard8

Depends on D144672

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 4: Explicitly specify import or imported symbol for importScripts in services/. r=Standard8

Depends on D144673

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 5: Explicitly specify import or imported symbol for importScripts in toolkit/components/lz4/. r=Standard8

Depends on D144674

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 6: Explicitly specify import or imported symbol for importScripts in toolkit/components/osfile/. r=Standard8

Depends on D144675

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 7: Explicitly specify import or imported symbol for importScripts in toolkit/components/promiseworker/. r=Standard8

Depends on D144676

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 8: Explicitly specify import or imported symbol for importScripts in toolkit/components/reader/. r=Standard8!

Depends on D144677

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 9: Explicitly specify import or imported symbol for importScripts in toolkit/components/thumbnails/. r=Standard8

Depends on D144678

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 10: Explicitly specify import or imported symbol for importScripts in toolkit/components/workerloader/. r=Standard8

Depends on D144679

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1766099 - Part 11: Do not use modules.json for importScripts. r=Standard8

Depends on D144680

Comment 18

[Tooru Fujisawa [:arai]]

Forgot to mention. the linter errors are expected for Parts 1-10, given the rule is fixed in Part 11, and until then both the modules.json and import-globals-from have effect.
If necessary, I'll fold them into single patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a158d8879ba
Part 1: Explicitly specify import or imported symbol for importScripts in browser/components/newtab/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/9b8506f0f227
Part 2: Explicitly specify import or imported symbol for importScripts in devtools/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/82dba376abff
Part 3: Explicitly specify import or imported symbol for importScripts in dom/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/f4899b6d4da4
Part 4: Explicitly specify import or imported symbol for importScripts in services/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/8018efd4ab2a
Part 5: Explicitly specify import or imported symbol for importScripts in toolkit/components/lz4/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/4b5d4a6e045b
Part 6: Explicitly specify import or imported symbol for importScripts in toolkit/components/osfile/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/0ad5df800908
Part 7: Explicitly specify import or imported symbol for importScripts in toolkit/components/promiseworker/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/980aae0780e9
Part 8: Explicitly specify import or imported symbol for importScripts in toolkit/components/reader/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/bdcc533f8928
Part 9: Explicitly specify import or imported symbol for importScripts in toolkit/components/thumbnails/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/abc4b50ba8f1
Part 10: Explicitly specify import or imported symbol for importScripts in toolkit/components/workerloader/. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/c7c865812f8f
Part 11: Do not use modules.json for importScripts. r=Standard8

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5a158d8879ba
https://hg.mozilla.org/mozilla-central/rev/9b8506f0f227
https://hg.mozilla.org/mozilla-central/rev/82dba376abff
https://hg.mozilla.org/mozilla-central/rev/f4899b6d4da4
https://hg.mozilla.org/mozilla-central/rev/8018efd4ab2a
https://hg.mozilla.org/mozilla-central/rev/4b5d4a6e045b
https://hg.mozilla.org/mozilla-central/rev/0ad5df800908
https://hg.mozilla.org/mozilla-central/rev/980aae0780e9
https://hg.mozilla.org/mozilla-central/rev/bdcc533f8928
https://hg.mozilla.org/mozilla-central/rev/abc4b50ba8f1
https://hg.mozilla.org/mozilla-central/rev/c7c865812f8f

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: Bug 1766099 - explicitly specify import or imported sybmol for importScripts in DNS.jsm. r=#thunderbird-reviewers

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ea09f89c7e0f
explicitly specify import or imported sybmol for importScripts in DNS.jsm. r=#thunderbird-reviewers,rjl