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

Prevent accidental use of globals #8340

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 14, 2020

Previously all browser globals were allowed to be used anywhere by ESLint because we had set the env property to browser in the ESLint config. This has made it easy to accidentally use browser globals (e.g. #8338), so it has been removed. Instead we now have a short list of allowed globals.

All browser globals are now accessed as properties on window.

Unfortunately this change resulted in a few different confusing unit test errors, as some of our unit tests setup assumed that a particular global would be used via window or global. In particular,
window.fetch didn't work correctly because it wasn't patched by the AbortController polyfill (only global.fetch was being patched). The jsdom-global package we were using complicated matters by setting all of the JSDOM window properties directly on global, overwriting the AbortController for example.

The helpers.js test setup module has been simplified somewhat by removing jsdom-global and constructing the JSDOM instance manually. The JSDOM window is set on window, and a few properties are set on global as well as needed by various dependencies. node-fetch and
the AbortController polyfill/patch now work as expected as well, though fetch is only available on window now.

@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 14, 2020

This PR is in draft because it depends upon #8336, #8337, #8338, and #8339. They've been cherry-picked into this branch for now; I'll rebase this once they're all merged.

Only the final commit (dfa937d82f20253aa6a9ec316a90e6510eff9cba) should be considered for review; the other commits are from the other PRs.

Edit: All of the mentioned PRs have been merged, and this branch has been rebased. It's now ready for review.

@metamaskbot
Copy link
Collaborator

Builds ready [dfa937d]
Page Load Metrics (678 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint37594253
domContentLoaded4297976767033
load4317986787033
domInteractive4287966767033

@Gudahtt Gudahtt force-pushed the prevent-accidental-use-of-globals branch from dfa937d to 8f7dd4c Compare April 15, 2020 15:39
@Gudahtt Gudahtt changed the base branch from develop to replace-mock-fetch April 15, 2020 15:39
@Gudahtt Gudahtt force-pushed the prevent-accidental-use-of-globals branch from 8f7dd4c to fedb1df Compare April 15, 2020 15:53
Previously all browser globals were allowed to be used anywhere by
ESLint because we had set the `env` property to `browser` in the ESLint
config. This has made it easy to accidentally use browser globals
(e.g. #8338), so it has been removed. Instead we now have a short list
of allowed globals.

All browser globals are now accessed as properties on `window`.

Unfortunately this change resulted in a few different confusing unit
test errors, as some of our unit tests setup assumed that a particular
global would be used via `window` or `global`. In particular,
`window.fetch` didn't work correctly because it wasn't patched by the
AbortController polyfill (only `global.fetch` was being patched).
The `jsdom-global` package we were using complicated matters by setting
all of the JSDOM `window` properties directly on `global`, overwriting
the `AbortController` for example.

The `helpers.js` test setup module has been simplified somewhat by
removing `jsdom-global` and constructing the JSDOM instance manually.
The JSDOM window is set on `window`, and a few properties are set on
`global` as well as needed by various dependencies. `node-fetch` and
the AbortController polyfill/patch now work as expected as well,
though `fetch` is only available on `window` now.
@Gudahtt Gudahtt force-pushed the prevent-accidental-use-of-globals branch from fedb1df to bffe288 Compare April 15, 2020 16:02
@Gudahtt Gudahtt changed the base branch from replace-mock-fetch to develop April 15, 2020 16:02
@Gudahtt Gudahtt marked this pull request as ready for review April 15, 2020 16:02
@Gudahtt Gudahtt requested a review from jennypollack as a code owner April 15, 2020 16:02
@metamaskbot
Copy link
Collaborator

Builds ready [bffe288]
Page Load Metrics (700 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint389254189
domContentLoaded36784569811053
load36984870011053
domInteractive36784569711053

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for cleaning this up!

@Gudahtt Gudahtt merged commit 5ee1291 into develop Apr 15, 2020
@Gudahtt Gudahtt deleted the prevent-accidental-use-of-globals branch April 15, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants