Add prefer-global-this rule

[axetroy]

Close #978

[fisker]

The current implementation uses the Identifier node for judgment.

However, this node appears in many cases, so it may cause misjudgment. Maybe this isn't a good way to do it.

We can use GlobalReferenceTracker or use ReferenceTracker directly from @eslint-community/eslint-utils

[axetroy]

/cc @fisker @fregante

[fregante]

The rule should accept and pass all known window properties, so it likely needs to import a list from npm.

Pass

window.innerHeight
window.addEventListener()
window.name = 'Future'

https://developer.mozilla.org/en-US/docs/Web/API/Window

[sindresorhus]

I agree, actual window properties should stay being window. since it's weird to get, for example, the innerHeight from globalThis.

[sindresorhus]

@fregante I don't think it can be an automatic list as https://developer.mozilla.org/en-US/docs/Web/API/Window lists a lot of non-window-specific properties too. For example, window.crypto should be globalThis.crypto.

[fregante]

These codes will still work after changing to globalThis

Interestingly, that appears to be true. I was under the impression that globalThis was not exactly window in browsers. Firefox for example returns false for 'chrome' in window in extensions (or at least it used to)

I don't think it can be an automatic list

As "automatic" as a list manually written and published by someone else, following the same meaning, that's what I meant 😃

But yeah the list isn't huge so a copy-paste from that page and manual exclusion isn't a huge problem, at least to get this started.

[sindresorhus]

Do I need to maintain this list in plugin, or a separate npm package?

I would prefer to maintain it here. The list should not be too long. Most properties on window. are just globals.

[sindresorhus]

You can probably find the main ones here: https://html.spec.whatwg.org/multipage/nav-history-apis.html#the-window-object

[fregante]

I can help filter out some of these.

From a first look, it varies:

• some are definitely not window properties, like the Screen constructor
• some are generally used without window. so they could be with globalThis or with nothing at all
• some are events that aren’t strictly related to the window, like onpaste (globalThis.onpaste might work just as well)

For the first two categories, they should either use globalThis or nothing at all (unless they’re used as globalThis.location?.href). So I wonder if instead of “prefer globalThis” this rule should just be “don’t use window”

edit: however this would make the logic more complicated, so never mind the rule name change suggestion.

[sindresorhus]

So I wonder if instead of “prefer globalThis” this rule should just be “don’t use window”

But it's "don't use window, or self, or global". I think "prefer globalThis" is easier to say.

[fregante]

What I was suggesting is that 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. It can be its own rule though.

Edit: I opened #2419

👌

[fisker]

Please try to reuse existing utilities.

[axetroy]

Let us know when this is ready for another review. Make sure #2410 (files) is resolved first.

I think we should ignore them, or simply ignore all computed properties.

@sindresorhus This is already resolved. see the test case https://github.com/sindresorhus/eslint-plugin-unicorn/pull/2410/files#diff-560654447874168910d7716e1da47d1f8f03eebfc3f5eaa14f7977133d1d1882R57-R64

[axetroy]

@fisker Could you please review it again and let me know if there is anything that needs to change

[zanminkian]

I did not reviewed this PR carefully. Inspired by the implementation of eslint built-in rule no-restricted-globals. I provide a simple demo to implement it.

{
  create: (context) => ({
    Program: (node) => {
      const banned = ["global", "self"];
      // Report variables declared elsewhere
      const scope = context.sourceCode.getScope(node);
      scope.variables.forEach((v) => {
        if (banned.includes(v.name)) {
          v.references.forEach((ref) => {
            context.report({node:ref.identifier, message: "xxxxxx"});
          });
        }
      });
      // Report variables not declared at all
      scope.through.forEach((ref) => {
        if (banned.includes(ref.identifier.name)) {
           context.report({node:ref.identifier, message: "xxxxxx"});
        }
      });
    },
  }),
}

[axetroy]

I did not reviewed this PR carefully. Inspired by the implementation of eslint built-in rule no-restricted-globals. I provide a simple demo to implement it.

{
  create: (context) => ({
    Program: (node) => {
      const banned = ["global", "self"];
      // Report variables declared elsewhere
      const scope = context.sourceCode.getScope(node);
      scope.variables.forEach((v) => {
        if (banned.includes(v.name)) {
          v.references.forEach((ref) => {
            context.report({node:ref.identifier, message: "xxxxxx"});
          });
        }
      });
      // Report variables not declared at all
      scope.through.forEach((ref) => {
        if (banned.includes(ref.identifier.name)) {
           context.report({node:ref.identifier, message: "xxxxxx"});
        }
      });
    },
  }),
}

Nice, it works without change any test case and the code is more concise

[zanminkian]

In this function, we should completely ban self and global, as they a not standard.

As for the window, if the api only works on browser, window can be used. Otherwise, window should be banned too.

[fisker]

Worker APIs should use self

[fregante]

Should they? I think self was chosen because there's no window, but globalThis should still be preferred there I think.

• Service workers introduction: 2014
• globalThis introduction: 2019

I think using self in service workers is more of a historical artifact rather than a necessity. We can discuss/change this later though, I don't want to hold this PR any longer 🙏

[sindresorhus]

@fregante Can you open an issue?

[fisker]

Great work, and sorry for the delay.

[sindresorhus]

Nice work, @axetroy 👍

[charles4221]

Just upgraded my version of the plugin and started getting lint errors for this rule on code like this:

export function detectPrefersDarkMode() {
  return (
    typeof window !== 'undefined' &&
    window.matchMedia('(prefers-color-scheme: dark)').matches
  );
}

Shouldn't this type of usage be excluded from this rule since globalThis.matchMedia will crash with a TypeError: globalThis.matchMedia is not a function?

[fregante]

It works for me. That function does exist in your browser. Are you sure that's not just your test env crashing?

You probably want to replace all that with: globalThis.matchMedia?.(etc).matches

[perrin4869]

So this rule is failing for me on this line:

https://github.com/dotcore64/react-stay-scrolled/blob/master/src%2Findex.js#L11

Since globalThis is defined in all environments, it can't be used to detect a browser environment or a different environment.

It would be cool if these edge cases could be handled

[fregante]

@perrin4869 see #2468

[stof]

why allowing self.location in workers but not window.location ?

[fregante]

See #2458