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

Add prefer-global-this rule #2410

Merged
merged 71 commits into from
Sep 29, 2024
Merged
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
b262df1
Add `prefer-global-this` rule
axetroy Jul 26, 2024
23b9b18
fix: add more test
axetroy Jul 26, 2024
63333a2
fix: lint
axetroy Jul 26, 2024
f2c4c6a
test: add more test case
axetroy Jul 26, 2024
cdace67
fix: lint
axetroy Jul 26, 2024
77d99d4
add test for typescript
axetroy Jul 26, 2024
957f54a
add more test
axetroy Jul 26, 2024
d7b725d
Re-implement prefer-global-this
axetroy Jul 26, 2024
e8f791f
fix: report multiple nodes
axetroy Jul 26, 2024
fc0a9be
Support more statement and add test case
axetroy Jul 26, 2024
6021d48
docs: update description
axetroy Jul 26, 2024
b91e840
fix: Allows access to window/worker platform-specific APIs directly
axetroy Jul 29, 2024
c26d60b
test: update test case
axetroy Jul 29, 2024
b60b2b3
docs: update README of rule
axetroy Jul 29, 2024
83419f1
test: update test case
axetroy Jul 29, 2024
b6e2890
test: add more test case
axetroy Jul 29, 2024
287ee97
docs: update readme
axetroy Jul 29, 2024
c2289b1
update apis
axetroy Jul 29, 2024
1368a75
Update prefer-global-this.js
sindresorhus Jul 29, 2024
9689cae
Update prefer-global-this.md
sindresorhus Jul 29, 2024
c8e4217
Update prefer-global-this.js
sindresorhus Jul 29, 2024
1543ced
Filter out some properties
fregante Jul 29, 2024
588e158
fix: test case
axetroy Jul 30, 2024
71fdcd8
Update test/prefer-global-this.mjs
axetroy Jul 30, 2024
27d7096
Update docs/rules/prefer-global-this.md
axetroy Jul 30, 2024
b9cc588
fix: eslint-doc-generator
axetroy Jul 30, 2024
fecdfff
docs: update docs
axetroy Jul 30, 2024
b258aa1
feat: report error in some event that dost not relative to window
axetroy Jul 30, 2024
45b5a99
Discard changes to .github/workflows/title-formatter.yml
fregante Jul 30, 2024
0bc477d
test: add test for jsdom
axetroy Jul 30, 2024
a7bbe9f
docs: update
axetroy Jul 30, 2024
7bb73ec
docs: Add description of windowSpecificAPIs
axetroy Jul 30, 2024
8be6891
docs: update
axetroy Jul 31, 2024
e15c213
refactor: improve the code
axetroy Jul 31, 2024
abaffc2
refactor: Simplify the MemberExpression's logic
axetroy Jul 31, 2024
ffc9b46
Update prefer-global-this.js
sindresorhus Jul 31, 2024
a105b89
reuse GlobalReferenceTracker to simplify the code
axetroy Aug 5, 2024
91cd643
re-trigger ci
axetroy Aug 5, 2024
8cc0c79
docs: update
axetroy Aug 5, 2024
8678b45
Update prefer-global-this.md
sindresorhus Aug 5, 2024
c83eb91
Remove unnecessary switch
axetroy Aug 5, 2024
cd0dacb
update prefer-global-this.md
axetroy Aug 6, 2024
dcaccbb
refactor: Replace `{{value}}` with `globalThis` in prefer-global-this.js
axetroy Aug 9, 2024
5e85ec3
refactor: Simplify window-specific API check in prefer-global-this.js
axetroy Aug 9, 2024
8fe496a
refactor: remove unnecessary condition in for of nodes
axetroy Aug 9, 2024
3b8e20f
chore: Remove unnecessary suggestion message in prefer-global-this.js
axetroy Aug 9, 2024
18e1f1b
test: add test case
axetroy Aug 9, 2024
453232b
refactor: use isShadowed to found shadowed variable
axetroy Aug 9, 2024
ac7a9b8
fix: eslint-docs
axetroy Aug 9, 2024
d5a63e7
test: add test case for AssignmentPattern
axetroy Aug 9, 2024
4bdfb10
update test case
axetroy Aug 15, 2024
d00d870
add test
axetroy Aug 19, 2024
45dd21c
Simplify
fisker Aug 19, 2024
0680206
Simplify
fisker Aug 19, 2024
56aed7e
Simplify
fisker Aug 19, 2024
13d9d89
Simplify logic
fisker Aug 19, 2024
29d89e5
Add tests
fisker Aug 19, 2024
1575cd8
fix: lint
axetroy Aug 20, 2024
75472c5
add test case for `foo[window]` and `foo[window.foo]`
axetroy Aug 22, 2024
2f83ffe
update test
axetroy Aug 24, 2024
b96dbe9
update docs
axetroy Aug 24, 2024
b76d424
Revert "Discard changes to .github/workflows/title-formatter.yml"
axetroy Aug 25, 2024
b5dc66f
Merge branch 'main' into prefer-global-this
axetroy Sep 10, 2024
640c84b
fix indent
axetroy Sep 10, 2024
c73d1e5
refactor: remove GlobalReferenceTracker and use scope lookup
axetroy Sep 10, 2024
a3a15bb
fix: did not detect the global var when not declared at globals options
axetroy Sep 11, 2024
12fe9a4
style jsdoc
axetroy Sep 11, 2024
84147cd
Revert tester change
fisker Sep 27, 2024
496483e
Simplify
fisker Sep 27, 2024
775288d
Minor tweak
fisker Sep 27, 2024
8922b37
Linting
fisker Sep 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions docs/rules/prefer-global-this.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# Prefer `globalThis` over `window`, `self`, and `global`

💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs).

🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

This rule will enforce the use of `globalThis` over `window`, `self`, and `global`.

However, there are several exceptions that remain permitted:

1. Certain window/WebWorker-specific APIs, such as `window.innerHeight` and `self.postMessage`
2. Window-specific events, such as `window.addEventListener('resize')`

The complete list of permitted APIs can be found in the rule's [source code](../../rules/prefer-global-this.js).

## Examples

```js
window; // ❌
globalThis; // ✅
```

```js
window.foo; // ❌
globalThis.foo; // ✅
```

```js
window[foo]; // ❌
globalThis[foo]; // ✅
```

```js
global; // ❌
globalThis; // ✅
```

```js
global.foo; // ❌
globalThis.foo; // ✅
```

```js
const {foo} = window; // ❌
const {foo} = globalThis; // ✅
```

```js
window.location; // ❌
globalThis.location; // ✅

window.innerWidth; // ✅ (Window specific API)
window.innerHeight; // ✅ (Window specific API)
```

```js
window.navigator; // ❌
globalThis.navigator; // ✅
```

```js
self.postMessage('Hello'); // ✅ (Web Worker specific API)
self.onmessage = () => {}; // ✅ (Web Worker specific API)
```

```js
window.addEventListener('click', () => {}); // ❌
globalThis.addEventListener('click', () => {}); // ✅

window.addEventListener('resize', () => {}); // ✅ (Window specific event)
window.addEventListener('load', () => {}); // ✅ (Window specific event)
window.addEventListener('unload', () => {}); // ✅ (Window specific event)
```
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
| [prefer-dom-node-text-content](docs/rules/prefer-dom-node-text-content.md) | Prefer `.textContent` over `.innerText`. | ✅ | | 💡 |
| [prefer-event-target](docs/rules/prefer-event-target.md) | Prefer `EventTarget` over `EventEmitter`. | ✅ | | |
| [prefer-export-from](docs/rules/prefer-export-from.md) | Prefer `export…from` when re-exporting. | ✅ | 🔧 | 💡 |
| [prefer-global-this](docs/rules/prefer-global-this.md) | Prefer `globalThis` over `window`, `self`, and `global`. | ✅ | 🔧 | |
| [prefer-includes](docs/rules/prefer-includes.md) | Prefer `.includes()` over `.indexOf()`, `.lastIndexOf()`, and `Array#some()` when checking for existence or non-existence. | ✅ | 🔧 | 💡 |
| [prefer-json-parse-buffer](docs/rules/prefer-json-parse-buffer.md) | Prefer reading a JSON file as a buffer. | | 🔧 | |
| [prefer-keyboard-event-key](docs/rules/prefer-keyboard-event-key.md) | Prefer `KeyboardEvent#key` over `KeyboardEvent#keyCode`. | ✅ | 🔧 | |
Expand Down
241 changes: 241 additions & 0 deletions rules/prefer-global-this.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
'use strict';

const {GlobalReferenceTracker} = require('./utils/global-reference-tracker.js');
const isShadowed = require('./utils/is-shadowed.js');

const MESSAGE_ID_ERROR = 'prefer-global-this/error';
const messages = {
[MESSAGE_ID_ERROR]: 'Prefer `globalThis` over `{{value}}`.',
};

const globalIdentifier = new Set(['window', 'self', 'global']);

const windowSpecificEvents = new Set([
'resize',
'blur',
'focus',
'load',
'scroll',
'scrollend',
'wheel',
'beforeunload', // Browsers might have specific behaviors on exactly `window.onbeforeunload =`
'message',
'messageerror',
'pagehide',
'pagereveal',
'pageshow',
'pageswap',
'unload',
]);

/**
Note: What kind of API should be a windows-specific interface?

1. It's directly related to window (✅ window.close())
2. It does NOT work well as globalThis.x or x (✅ window.frames, window.top)

Some constructors are occasionally related to window (like Element !== iframe.contentWindow.Element), but they don't need to mention window anyway.

Please use these criteria to decide whether an API should be added here. Context: https://github.com/sindresorhus/eslint-plugin-unicorn/pull/2410#discussion_r1695312427
*/
const windowSpecificAPIs = new Set([
// Properties and methods
// https://html.spec.whatwg.org/multipage/nav-history-apis.html#the-window-object
'name',
'locationbar',
'menubar',
'personalbar',
'scrollbars',
'statusbar',
'toolbar',
'status',
'close',
'closed',
'stop',
'focus',
'blur',
'frames',
'length',
'top',
'opener',
'parent',
'frameElement',
'open',
'originAgentCluster',
'postMessage',

// Events commonly associated with "window"
...[...windowSpecificEvents].map(event => `on${event}`),

// To add/remove/dispatch events that are commonly associated with "window"
// https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-flow
'addEventListener',
'removeEventListener',
'dispatchEvent',
axetroy marked this conversation as resolved.
Show resolved Hide resolved

// https://dom.spec.whatwg.org/#idl-index
fregante marked this conversation as resolved.
Show resolved Hide resolved
'event', // Deprecated and quirky, best left untouched

// https://drafts.csswg.org/cssom-view/#idl-index
'screen',
'visualViewport',
'moveTo',
'moveBy',
'resizeTo',
'resizeBy',
'innerWidth',
'innerHeight',
'scrollX',
'pageXOffset',
'scrollY',
'pageYOffset',
'scroll',
'scrollTo',
'scrollBy',
'screenX',
'screenLeft',
'screenY',
'screenTop',
'screenWidth',
'screenHeight',
'devicePixelRatio',
]);

const webWorkerSpecificAPIs = new Set([
// https://html.spec.whatwg.org/multipage/workers.html#the-workerglobalscope-common-interface
'addEventListener',
'removeEventListener',
'dispatchEvent',

'self',
'location',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2458

'navigator',
'onerror',
'onlanguagechange',
'onoffline',
'ononline',
'onrejectionhandled',
'onunhandledrejection',
fregante marked this conversation as resolved.
Show resolved Hide resolved

// https://html.spec.whatwg.org/multipage/workers.html#dedicated-workers-and-the-dedicatedworkerglobalscope-interface
'name',
'postMessage',
'onconnect',
]);

/**
Report the node with a message.

@param {import('estree').Node} node
*/
function getProblem(node) {
return {
node,
messageId: MESSAGE_ID_ERROR,
data: {value: node.name},
fix: (/** @type {import('eslint').Rule.RuleFixer} fixer */ fixer) => fixer.replaceText(node, 'globalThis'),
};
}

/**
Handle nodes and check if they should be reported.

@param {import('eslint').Rule.RuleContext} context
@param {import('estree').Node} node
*/
function handleNode(context, node) {
if (node.type === 'Identifier' && globalIdentifier.has(node.name) && !isShadowed(context.sourceCode.getScope(node), node)) {
return getProblem(node);
}
}

/**
Check if the node is a window-specific API.

@param {import('estree').MemberExpression} node
@returns {boolean}
*/
const isWindowSpecificAPI = node => {
if (node.type !== 'MemberExpression') {
return false;
}

if (node.object.name !== 'window' || node.property.type !== 'Identifier') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need add !node.computed here, don't forgot to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think !node.computed should not be added here, or am I missing some case?.

Regardless of whether it is a computed property or not, this rule should be applied.

There is the test that we already have

window['title'] // node.computed === true
window.title    // node.computed === false
## invalid(4): window.foo

> Input

    `␊
      1 | window.foo␊
    `

> Output

    `␊
      1 | globalThis.foo␊
    `

> Error 1/1

    `␊
    > 1 | window.foo␊
        | ^^^^^^ Prefer \`globalThis\` over \`window\`.␊
    `

## invalid(5): window[foo]

> Input

    `␊
      1 | window[foo]␊
    `

> Output

    `␊
      1 | globalThis[foo]␊
    `

> Error 1/1

    `␊
    > 1 | window[foo]␊
        | ^^^^^^ Prefer \`globalThis\` over \`window\`.␊
    `

## invalid(6): window["foo"]

> Input

    `␊
      1 | window["foo"]␊
    `

> Output

    `␊
      1 | globalThis["foo"]␊
    `

> Error 1/1

    `␊
    > 1 | window["foo"]␊
        | ^^^^^^ Prefer \`globalThis\` over \`window\`.␊
    `

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window[title]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no difference between window[title] and window[foo]

Both title and foo are variable accesses. test added.

## invalid(6): window[title]

> Input

    `␊
      1 | window[title]␊
    `

> Output

    `␊
      1 | globalThis[title]␊
    `

> Error 1/1

    `␊
    > 1 | window[title]␊
        | ^^^^^^ Prefer \`globalThis\` over \`window\`.␊

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window[name] doesn't mean it's accessing window.name, since name can be other values.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not checking window is MemberExpression.object either, foo[window]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't click resolve button, I can't find my comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not checking window is MemberExpression.object either, foo[window]

The test added in 4be4699

return false;
}

if (windowSpecificAPIs.has(node.property.name)) {
if (['addEventListener', 'removeEventListener', 'dispatchEvent'].includes(node.property.name) && node.parent.type === 'CallExpression' && node.parent.callee === node) {
const argument = node.parent.arguments[0];
return argument && argument.type === 'Literal' && windowSpecificEvents.has(argument.value);
}

return true;
}

return false;
};

/**
@param {import('estree').Node} node
@param {import('estree').Identifier} identifier
@returns
*/
function isComputedMemberExpression(node, identifier) {
return node.type === 'MemberExpression' && node.computed && node.object === identifier;
}

/**
Check if the node is a web worker specific API.

@param {import('estree').MemberExpression} node
@returns {boolean}
*/
const isWebWorkerSpecificAPI = node => node.type === 'MemberExpression' && node.object.name === 'self' && node.property.type === 'Identifier' && webWorkerSpecificAPIs.has(node.property.name);

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
const tracker = new GlobalReferenceTracker({
objects: globalIdentifier,
handle(reference) {
const {node} = reference;

if (node.type !== 'Identifier'
|| isWindowSpecificAPI(node.parent)
|| isWebWorkerSpecificAPI(node.parent)
|| isComputedMemberExpression(node.parent, node)
) {
return;
}

return getProblem(node);
},
});

return {
...tracker.createListeners(context),
/** @param {import('estree').AssignmentExpression} node */
AssignmentExpression(node) {
return handleNode(context, node.left);
},
/** @param {import('estree').UpdateExpression} node */
UpdateExpression(node) {
axetroy marked this conversation as resolved.
Show resolved Hide resolved
return handleNode(context, node.argument);
},
};
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
description: 'Prefer `globalThis` over `window`, `self`, and `global`.',
recommended: true,
},
fixable: 'code',
hasSuggestions: false,
messages,
},
};
1 change: 1 addition & 0 deletions test/package.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const RULES_WITHOUT_PASS_FAIL_SECTIONS = new Set([
// Intended to not use `pass`/`fail` section in this rule.
'prefer-modern-math-apis',
'prefer-math-min-max',
'prefer-global-this',
]);

test('Every rule is defined in index file in alphabetical order', t => {
Expand Down
Loading