Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule proposal: Avoid or prefer using globals as globalThis.something #2419

Open
fregante opened this issue Aug 1, 2024 · 6 comments
Open

Comments

@fregante
Copy link
Collaborator

fregante commented Aug 1, 2024

Description

Originally posted in #2410 (comment)

you should still prefer nothing at all over globalThis in some cases (unless you're using ?.), like you wouldn't use new globalThis.Image() or globalThis.Array.from() or globalThis.globalThis

I'd lean towards never using globalThis (and window, self, global, etc) for known globals. globalThis.jQuery = jQuery should still be allowed though.

Fail

globalThis.alert();
globalThis.fetch();
globalThis.console.log();
globalThis.addEventListener('navigate', fn);
globalThis.JSON.stringify();
globalThis.window.self.close()
new globalThis.Image();
globalThis.RegExp.$1;

Pass

alert();
fetch();
console.log();
addEventListener('navigate', fn);
JSON.stringify();
self.close(); // Partial exception
new Image();
RegExp.$1;

The exception is the same as the one described in #2410 (comment)

Proposed rule name

global-properties or just avoid-global-propertoes

Additional Info

No response

@fregante
Copy link
Collaborator Author

fregante commented Aug 1, 2024

Drawbacks/exceptions:

// Assuming Node.js
const log = globalThis.alert; // undefined
const log = alert; // Uncaught ReferenceError: alert is not defined

If the function is called, this makes no difference:

// Assuming Node.js
globalThis.alert(); // Uncaught ReferenceError: alert is not defined
alert();            // Uncaught ReferenceError: alert is not defined

new globalThis.Image(); // Uncaught ReferenceError: Image is not defined
new Image();            // Uncaught ReferenceError: Image is not defined

@sindresorhus
Copy link
Owner

Name: no-unnecessary-globalthis?

@sindresorhus
Copy link
Owner

Not sure about this. I do agree that it's nice to avoid it in browser-only contexts, but for cross-platform code, it's necessary.

We could ignore cases like globalThis.alert?.(), but there are many ways to use APIs that may or may not be available, and I worry that this rule will be annoying.

Maybe we could exclude it from the recommended preset and in XO, only enable it when the environment is browser-only?

@fregante
Copy link
Collaborator Author

fregante commented Aug 1, 2024

Name: no-unnecessary-globalthis?

I suppose that focusing on globalThis simplifies the logic and leaves window-specific exclusions to prefer-global-this:

window.self.alert();     // ❌ `prefer-global-this`
globalThis.self.alert(); // ❌ `no-unnecessary-globalthis`
self.alert();            // ❌ `prefer-global-this`
globalThis.alert();      // ❌ `no-unnecessary-globalthis`
alert(); 

it's necessary

See my second comment, in some cases it's indeed required, but in others it doesn't make a difference.

globalThis.alert?.() would indeed have to be an exception since alert?.() won't catch reference errors. Related:

when the environment is browser-only?

That's what I thought too, but globalThis.fetch() and globalThis.atob() is also available on Node.

So, safe to replace:

  • globals that are part of ES
  • values that are accessed/called without ?.
    • globalThis.location.href, new globalThis.Element()

Unsafe to replace:

  • platform-specific globals that are unused
    • const fn = globalThis.alert; -> Function in browser, undefined in Node
  • platform-specific globals that are followed by ?.
    • globalThis.alert?.()

@dimaMachina
Copy link
Contributor

Maybe similar proposal #1959

@fregante
Copy link
Collaborator Author

fregante commented Aug 1, 2024

👍 Yeah the core is the same, but this request is more generic, including node. Some of the comments are also covered by another rule:

So I'll close that in favor of this and prefer-global-this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants