test: use strict mode in global setters test #56742
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
test-global-setters.js was added in nodejs#26882 and hasn't been modified since. It appears that it avoids strict mode so that unscoped identifiers are treated as globals, but that's true in strict mode too. (In sloppy mode, an assignment to an undefined identifier will create a global, but that's not what this test does. In strict mode, those assignments will throw an error, which would seem to be what we would want.)
nodejs-github-bot
added
needs-ci
|
Pinging author-of-the-test @guybedford for review in case there's something I'm misunderstanding. I'll also add |
github-actions
bot
removed
the
request-ci
lpinca
approved these changes
Jan 24, 2025
jasnell
reviewed
Jan 24, 2025
jasnell
approved these changes
Jan 24, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56742 +/- ##
==========================================
- Coverage 89.21% 89.21% -0.01%
==========================================
Files 662 662
Lines 191968 191976 +8
Branches 36955 36953 -2
==========================================
- Hits 171269 171263 -6
- Misses 13535 13555 +20
+ Partials 7164 7158 -6 |
Trott
added
commit-queue-squash
github-actions
bot
removed
the
request-ci
aduh95
added
author ready
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56742 ✔ Done loading data for nodejs/node/pull/56742 ----------------------------------- PR info ------------------------------------ Title test: use strict mode in global setters test (#56742) Author Rich Trott <rtrott@gmail.com> (@Trott) Branch Trott:strict -> nodejs:main Labels test, author ready, needs-ci, commit-queue-squash Commits 2 - test: use strict mode in global setters test - squash! test: use strict mode in global setters test Committers 1 - Rich Trott <rtrott@gmail.com> PR-URL: https://github.com/nodejs/node/pull/56742 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56742 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - squash! test: use strict mode in global setters test ℹ This PR was created on Fri, 24 Jan 2025 14:45:39 GMT ✔ Approvals: 2 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/56742#pullrequestreview-2573031130 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56742#pullrequestreview-2573037493 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-01-24T17:42:01Z: https://ci.nodejs.org/job/node-test-pull-request/64722/ - Querying data for job/node-test-pull-request/64722/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/12975708665 |
guybedford
approved these changes
Jan 26, 2025
Comment on lines
+1
to
+2
| // When setters and getters were added for global.process and global.Buffer to | ||
| // create a deprecation path for them in ESM, this test was added to make sure |
There was a problem hiding this comment.
It could be worth noting at this point it's no longer about creating a deprecation path since that ship has likely sailed short of a very big breaking change, but that these getters and setters may be used to attenuate access if we gain the ability to gate the global object by module accessor in future somehow.
Trott
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 1263efd |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test-global-setters.js was added in
#26882 and hasn't been modified since. It appears that it avoids strict mode so that unscoped identifiers are treated as globals, but that's true in strict mode too. (In sloppy mode, an assignment to an undefined identifier will create a global, but that's not what this test does. In strict mode, those assignments will throw an error, which would seem to be what we would want.)