-
-
Notifications
You must be signed in to change notification settings - Fork 412
Add prefer-global-this
rule
#2410
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
Changes from 67 commits
b262df1
23b9b18
63333a2
f2c4c6a
cdace67
77d99d4
957f54a
d7b725d
e8f791f
fc0a9be
6021d48
b91e840
c26d60b
b60b2b3
83419f1
b6e2890
287ee97
c2289b1
1368a75
9689cae
c8e4217
1543ced
588e158
71fdcd8
27d7096
b9cc588
fecdfff
b258aa1
45b5a99
0bc477d
a7bbe9f
7bb73ec
8be6891
e15c213
abaffc2
ffc9b46
a105b89
91cd643
8cc0c79
8678b45
c83eb91
cd0dacb
dcaccbb
5e85ec3
8fe496a
3b8e20f
18e1f1b
453232b
ac7a9b8
d5a63e7
4bdfb10
d00d870
45dd21c
0680206
56aed7e
13d9d89
29d89e5
1575cd8
75472c5
2f83ffe
b96dbe9
b76d424
b5dc66f
640c84b
c73d1e5
a3a15bb
12fe9a4
84147cd
496483e
775288d
8922b37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,220 @@ | ||
'use strict'; | ||
|
||
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', | ||
|
||
// 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why allowing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
]); | ||
|
||
/** | ||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 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\`.␊
` There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no difference between Both ## invalid(6): window[title]
> Input
`␊
1 | window[title]␊
`
> Output
`␊
1 | globalThis[title]␊
`
> Error 1/1
`␊
> 1 | window[title]␊
| ^^^^^^ Prefer \`globalThis\` over \`window\`.␊ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't click There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 | ||
@param {import('estree').Identifier} node | ||
*/ | ||
function reportProblem(context, node) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this function, we should completely ban As for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worker APIs should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should they? I think
I think using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fregante Can you open an issue? |
||
if (isWindowSpecificAPI(node.parent) || isWebWorkerSpecificAPI(node.parent) || isComputedMemberExpression(node.parent, node)) { | ||
fisker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
|
||
context.report({ | ||
node, | ||
messageId: MESSAGE_ID_ERROR, | ||
data: {value: node.name}, | ||
fix: fixer => fixer.replaceText(node, 'globalThis'), | ||
}); | ||
} | ||
|
||
/** @param {import('eslint').Rule.RuleContext} context */ | ||
const create = context => ({ | ||
Program(programNode) { | ||
const scope = context.sourceCode.getScope(programNode); | ||
|
||
// Report variables declared at globals options | ||
for (const variable of scope.variables) { | ||
fisker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (globalIdentifier.has(variable.name)) { | ||
for (const reference of variable.references) { | ||
reportProblem(context, reference.identifier); | ||
} | ||
} | ||
} | ||
|
||
// Report variables not declared at globals options | ||
for (const reference of scope.through) { | ||
if (globalIdentifier.has(reference.identifier.name)) { | ||
reportProblem(context, reference.identifier); | ||
} | ||
} | ||
}, | ||
}); | ||
|
||
/** @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, | ||
}, | ||
}; |
Uh oh!
There was an error while loading. Please reload this page.