Can we drop modules.json dependency of importScript ?
Categories
(Developer Infrastructure :: Lint and Formatting, task)
Tracking
(firefox101 fixed)
| Tracking | Status | |
|---|---|---|
| firefox101 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(12 files, 1 obsolete file)
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review |
modules.json is used for the following 3 purpose:
ChromeUtils.import, importing intothisorglobalThisCu.import(Components.utils.import), importing intothisorglobalThisimportScripts, 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-fromabove eachimportScripts - (c) Require
globalabove eachimportScripts
(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.
|
Assignee | |
Comment 1•2 years ago
|
||
Tried the combination of (a) and (b), and that looks reasonably small change
|
||
Comment 2•2 years ago
|
||
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.
|
|
Assignee | |
Comment 3•2 years ago
|
||
(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?)
|
|
Assignee | |
Comment 4•2 years ago
|
||
(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 ;)
|
|
Assignee | |
Comment 5•2 years ago
•
|
||
Here's WIP patch for option (b).
(actually 11 patches locally, for each directory https://treeherder.mozilla.org/jobs?repo=try&revision=21e48e3eacfcbe2a9a1e6f06bfd8b856cdc7e1d3
).
|
|
Assignee | |
Updated•2 years ago
|
|
|
||
Comment 6•2 years ago
|
||
Yeah, I think that's reasonable given our current infrastructure.
|
|
Assignee | |
Comment 7•2 years ago
|
||
|
|
Assignee | |
Comment 8•2 years ago
|
||
Depends on D144671
|
|
Assignee | |
Comment 9•2 years ago
|
||
Depends on D144672
|
|
Assignee | |
Comment 10•2 years ago
|
||
Depends on D144673
|
|
Assignee | |
Comment 11•2 years ago
|
||
Depends on D144674
|
|
Assignee | |
Comment 12•2 years ago
|
||
Depends on D144675
|
|
Assignee | |
Comment 13•2 years ago
|
||
Depends on D144676
|
|
Assignee | |
Comment 14•2 years ago
|
||
Depends on D144677
|
|
Assignee | |
Comment 15•2 years ago
|
||
Depends on D144678
|
|
Assignee | |
Comment 16•2 years ago
|
||
Depends on D144679
|
|
Assignee | |
Comment 17•2 years ago
|
||
Depends on D144680
|
|
Assignee | |
Updated•2 years ago
|
|
|
Assignee | |
Comment 18•2 years ago
|
||
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.
|
||
Updated•2 years ago
|
|
||
Comment 19•2 years ago
|
||
Pushed by arai_a@mac.com: 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•2 years ago
|
||
| bugherder | ||
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•2 years ago
|
||
|
|
||
Comment 22•2 years ago
|
||
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/ea09f89c7e0f explicitly specify import or imported sybmol for importScripts in DNS.jsm. r=#thunderbird-reviewers,rjl
|
||
Updated•2 years ago
|
Description
•