sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
4.25k stars 367 forks source link

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

Open fregante opened 2 months ago

fregante commented 2 months ago

Description

Originally posted in https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2410#issuecomment-2255792543

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 https://github.com/sindresorhus/eslint-plugin-unicorn/pull/2410#discussion_r1695312427

Proposed rule name

global-properties or just avoid-global-propertoes

Additional Info

No response

fregante commented 2 months ago

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 commented 2 months ago

Name: no-unnecessary-globalthis?

sindresorhus commented 2 months ago

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 commented 2 months ago

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:

Unsafe to replace:

dimaMachina commented 2 months ago

Maybe similar proposal https://github.com/sindresorhus/eslint-plugin-unicorn/issues/1959

fregante commented 2 months ago

👍 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