-
Notifications
You must be signed in to change notification settings - Fork 64
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
chore: replace 'global' with 'globalThis' #701
chore: replace 'global' with 'globalThis' #701
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
3f8b15e
to
2b33d6e
Compare
2b33d6e
to
d331922
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #701 +/- ##
=======================================
Coverage 83.41% 83.41%
=======================================
Files 40 40
Lines 1073 1073
Branches 197 197
=======================================
Hits 895 895
Misses 166 166
Partials 12 12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. LGTM!
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@@ -3,6 +3,7 @@ const { getBaseConfig } = require('@openedx/frontend-build'); | |||
|
|||
const config = getBaseConfig('eslint'); | |||
|
|||
config.globals = { globalThis: 'readonly' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradenmacdonald [curious] Lots of repos use global
today; I'm wondering if we should update the base ESLint config defined in @openedx/frontend-build
so each consuming shared JS library and/or MFE doesn't need to specify this config across many different repos. Curious your two cents.
On a similar note, I wonder if it'd be more acceptable to extend the shared ESLint config's env
option to include es2020
so that other similar syntax is deemed acceptable as well? @openedx/frontend-build
relies on the env
defined in @edx/eslint-config
here. Setting es2020
in env
would in place of configuring the globals
for globalThis
explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm definitely in favor of setting es2020
in env
as a default!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradenmacdonald Opened a PR to the base shared ESLint configuration (@edx/eslint-config
): openedx/eslint-config#153
I believe having es2020
in there would eliminate the need to keep es6
, since the environments (as far as I can tell) are cumulative/additive, where a later environment supersedes an earlier one. That said, I'm opting to keep es6
in there for now just in case that is not true.
Let me know your thoughts; will also share in FWG Slack :)
The
global
keyword is a non-standard NodeJS specific keyword. But since 2019, we can useglobalThis
instead, which works in every runtime. For example,globalThis
has the same consistent meaning in both browsers and NodeJS, whereasglobal
does not work in browsers unless you use some build process to rewrite it towindow
.Details: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#description
Example from Chrome console: