Skip to content

feat(confusing-browser-globals): Add relevant messaging to no-restricted-globals eslint rule #10714

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+94 −61
Open

feat(confusing-browser-globals): Add relevant messaging to no-restricted-globals eslint rule #10714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
feat(confusing-browser-globals): Add relevant messaging to no-restric… 
…ted-globals eslint rule

BREAKING CHANGE: export from confusing-browser-globals package changed from String[] to Object[] - no change required if used in eslint no-restricted-globals config
  • Loading branch information
@yanneves
yanneves committed Mar 19, 2021
commit cb45333c03da6d4002ca1d9f83b9d545168a5f6a
127 changes: 68 additions & 59 deletions packages/confusing-browser-globals/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,63 +7,72 @@

'use strict';

module.exports = [
'addEventListener',
'blur',
'close',
'closed',
'confirm',
'defaultStatus',
'defaultstatus',
'event',
'external',
'find',
'focus',
'frameElement',
'frames',
'history',
'innerHeight',
'innerWidth',
'length',
'location',
'locationbar',
'menubar',
'moveBy',
'moveTo',
'name',
'onblur',
'onerror',
'onfocus',
'onload',
'onresize',
'onunload',
'open',
'opener',
'opera',
'outerHeight',
'outerWidth',
'pageXOffset',
'pageYOffset',
'parent',
'print',
'removeEventListener',
'resizeBy',
'resizeTo',
'screen',
'screenLeft',
'screenTop',
'screenX',
'screenY',
'scroll',
'scrollbars',
'scrollBy',
'scrollTo',
'scrollX',
'scrollY',
'self',
'status',
'statusbar',
'stop',
'toolbar',
'top',
const restrictedGlobals = [
['addEventListener', 'document'],
['blur', 'window'],
['close', 'window'],
['closed', 'window'],
['confirm', 'window'],
['defaultStatus', 'none'],
['defaultstatus', 'none'],
['event', 'none'],
['external', 'none'],
['find', 'none'],
['focus', 'window'],
['frameElement', 'window'],
['frames', 'window'],
['history', 'window'],
['innerHeight', 'window'],
['innerWidth', 'window'],
['length', 'window'],
['location', 'window'],
['locationbar', 'window'],
['menubar', 'window'],
['moveBy', 'window'],
['moveTo', 'window'],
['name', 'window'],
['onblur', 'window'],
['onerror', 'window'],
['onfocus', 'window'],
['onload', 'window'],
['onresize', 'window'],
['onunload', 'window'],
['open', 'window'],
['opener', 'window'],
['opera', 'window'],
['outerHeight', 'window'],
['outerWidth', 'window'],
['pageXOffset', 'window'],
['pageYOffset', 'window'],
['parent', 'window'],
['print', 'window'],
['removeEventListener', 'document'],
['resizeBy', 'window'],
['resizeTo', 'window'],
['screen', 'window'],
['screenLeft', 'window'],
['screenTop', 'window'],
['screenX', 'window'],
['screenY', 'window'],
['scroll', 'window'],
['scrollbars', 'window'],
['scrollBy', 'window'],
['scrollTo', 'window'],
['scrollX', 'window'],
['scrollY', 'window'],
['self', 'WorkerGlobalScope'],
Copy link

Choose a reason for hiding this comment

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

This PR would be useful! It has my 👍 .

self is a property on the WorkerGlobalScope instance (it isn't static), so WorkerGlobalScope.self actually returns undefined.

A couple of alternatives to using self are: globalThis (older browsers not supported) and this (definitely not recommended). Personally, I believe self is better than either of these.

['status', 'window'],
['statusbar', 'window'],
['stop', 'window'],
['toolbar', 'window'],
['top', 'window'],
];

module.exports = restrictedGlobals.map(([name, parent]) => {
const message =
parent === 'none'
? 'Use local parameter instead.'
: `Use local parameter or ${parent}.${name} instead.`;

return { name, message };
});
28 changes: 26 additions & 2 deletions packages/confusing-browser-globals/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,30 @@ it('should return an Array of globals', () => {
expect(Array.isArray(globals)).toBe(true);
});

it('should contain "event" variable', () => {
expect(globals).toContain('event');
it('should contain "event" variable preferring local parameter', () => {
expect(globals).toContainEqual({
name: 'event',
message: 'Use local parameter instead.',
});
});

it('should contain "location" variable preferring local parameter or window global', () => {
expect(globals).toContainEqual({
name: 'location',
message: 'Use local parameter or window.location instead.',
});
});

it('should contain "addEventListener" variable preferring local parameter or document global', () => {
expect(globals).toContainEqual({
name: 'addEventListener',
message: 'Use local parameter or document.addEventListener instead.',
});
});

it('should contain "self" variable preferring local parameter or WorkerGlobalScope global', () => {
expect(globals).toContainEqual({
name: 'self',
message: 'Use local parameter or WorkerGlobalScope.self instead.',
});
});