Skip to content

[eslint-config-kibana] expand list of restricted globals - #15798

Merged
spalger merged 1 commit into
elastic:masterfrom
spalger:enhance/restrict-unexpected-globals
Jan 3, 2018
Merged

spalger merged 1 commit into
elastic:masterfrom
spalger:enhance/restrict-unexpected-globals

Conversation

@spalger

@spalger spalger commented Dec 29, 2017 •

Copy link
Copy Markdown
Contributor

I have long struggled with the fact that variables like error and name are actually globals in the browser, and when using env.browser: true in eslint uninitialized variables with those names are assumed to be references to window.error and window.name. Did you also know that status is a global? That just bit me in e5206ed...

This change to the eslint-config-kibana package defines 57 globals that should not be used, courtesy of create-react-app.

These globals are all still available if you use window.{global}, but with this change eslint will tell you when you're using the global name variable instead of the local one you might think you're using.

@spalger spalger added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// review v6.2.0 v7.0.0 labels Dec 29, 2017
@spalger
spalger requested review from kimjoar and rhoboat December 29, 2017 22:43

@kimjoar kimjoar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is great! LGTM

@rhoboat rhoboat left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you please double-(triple?)-check the casing of the restricted globals?

'closed',
'confirm',
'defaultStatus',
'defaultstatus',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dost my eyes deceive me? I see two of the same line here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh, one is capital S and one is lowercase s. Huh. Why this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rhoboat rhoboat left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nevermind, LGTM. I see that we're using the list directly from create-react-app. 👍

'closed',
'confirm',
'defaultStatus',
'defaultstatus',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@spalger
spalger merged commit 28663f6 into elastic:master Jan 3, 2018
@spalger spalger removed the v6.2.0 label Jan 3, 2018
@spalger

spalger commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

7.0/master: e4edbae
6.2/6.x: 8b1cc5a

@spalger
spalger deleted the enhance/restrict-unexpected-globals branch January 3, 2018 18:01
@spalger spalger added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 22, 2019
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-issue Indicates to automation that a pull request should not appear in the release notes review Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants