Closed Bug 1608278 Opened 4 years ago Closed 4 years ago

Stop assigning properties to the global `this` in JSMs - Round 1

Categories

(Firefox :: General, task, P5)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

For example this.LoginBreaches = { ... }: https://searchfox.org/mozilla-central/rev/ba4fab1cc2f1c9c4e07cdb71542b8d441707c577/browser/components/aboutlogins/LoginBreaches.jsm#25.

This should generally work by changing to const LoginBreaches = { ... }

See Also: → 1608281
Summary: Stop assigning properties to `this` in JSMs → Stop assigning properties to the global `this` in JSMs
Priority: -- → P5
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Depends on: 1609271

Why don't you use globalThis.LoginBreaches = { ... }?
It is great to reduce global environment pollution, but is it really tied to migration to ES modules?

(In reply to Masatoshi Kimura [:emk] from comment #1)

Why don't you use globalThis.LoginBreaches = { ... }?
It is great to reduce global environment pollution, but is it really tied to migration to ES modules?

Not sure. Ted, do you have an opinion?

FWIW I have a script prepared to remove this that handles most cases I've found so far, although it's complicated by consumers that use Cu.import or ChromeUtils.import(path, null) which then expect things to have been set explicitly on this. I'm not sure if using globalThis will let those cases continue to work in the meantime, but if so that might be an option as a stopgap instead of waiting on those cleanups to happen first.

Flags: needinfo?(tcampbell)
Depends on: 1608281

(In reply to Masatoshi Kimura [:emk] from comment #1)

Why don't you use globalThis.LoginBreaches = { ... }?
It is great to reduce global environment pollution, but is it really tied to migration to ES modules?

This would expose it across all modules I believe. The intent should be to keep modules self-contained and share via exports not on the shared global.

This patch was generated with the script https://github.com/bgrins/jsm-rewrites/blob/d2bbd6c459294b65955442e45b5a7f5dba11e639/no-this-property-assign.js
using the following command:

cp .gitignore .rgignore && rg -l -g '*.jsm' '' devtools | jscodeshift --stdin --transform ~/Code/jsm-rewrites/no-this-property-assign.js && ./mach eslint --outgoing --fix

There's also a manual fixup in Loader.jsm from const to var for a couple exports.

Yeah, using globalThis will expose to all modules. This seems like reasonable behaviour and is how things would be if you made a single-page-app with a bunch of ES modules.

Flags: needinfo?(tcampbell)

(In reply to Dave Townsend [:mossop] (he/him) from comment #3)

(In reply to Masatoshi Kimura [:emk] from comment #1)

Why don't you use globalThis.LoginBreaches = { ... }?
It is great to reduce global environment pollution, but is it really tied to migration to ES modules?

This would expose it across all modules I believe. The intent should be to keep modules self-contained and share via exports not on the shared global.

Currently, modules are not self-contained because using this will expose things to shared JSM globals, i.e. across all modules. If the intent is to make modules self-contained, this bug should not block esm-ification. Because we can migrate to ES modules using globalThis. Although it is status quo, it does not make things worse.

We need to stop relying on the global this in order to support ES Modules.
This instance was harder to fix by a script than others since it exposed two methods,
so this patch is a manual change.

(In reply to Masatoshi Kimura [:emk] from comment #6)

(In reply to Dave Townsend [:mossop] (he/him) from comment #3)

(In reply to Masatoshi Kimura [:emk] from comment #1)

Why don't you use globalThis.LoginBreaches = { ... }?
It is great to reduce global environment pollution, but is it really tied to migration to ES modules?

This would expose it across all modules I believe. The intent should be to keep modules self-contained and share via exports not on the shared global.

Currently, modules are not self-contained because using this will expose things to shared JSM globals, i.e. across all modules. If the intent is to make modules self-contained, this bug should not block esm-ification. Because we can migrate to ES modules using globalThis. Although it is status quo, it does not make things worse.

It was my understanding that modules are self-contained and this is not shared across JSMs. I believe that in the B2G days we had a memory saving by using the same global for all JSMs which is why we started assigning things to this which was made unique for each JSM but I think even that has gone away now. In tests I do not seem to be able to access anything from a different module from the one currently running.

A top-level this in a JSM is isolated to just that JSM, while globalThis is not. In ES modules, a top-level this is undefined and globalThis is shared.

We need to stop relying on the global this in order to support ES Modules.
In this case we have this.DER (which is exported) and class DER in the
same module.

Because of this, changing this.DER to const DER would lead to an error.
So this change renames the class to avoid the conflict.

We need to stop relying on the global this in order to support ES Modules.
This instance was harder to fix by a script than others since it's in a try/catch,
so this patch is a manual change.

Keywords: leave-open
Summary: Stop assigning properties to the global `this` in JSMs → Stop assigning properties to the global `this` in JSMs - Round 1
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9ce34f4166a
Automated rewrite away from assigning properties to `this` in JSM files in devtools;r=jdescottes
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/809fd7bbf052
Remove global `this` setter for LOG_LEVEL in Credentials.jsm r=eoger
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/771d32d2e850
Export DER as DERDecoder to avoid duplicate DER global in DER.jsm r=keeler
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79514ae28ef4
Declare clearTimeout and clearInterval as locally scoped variables in Timer.jsm r=mconley
Blocks: 1610653
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
No longer depends on: 1608281, 1609271
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.