lib: fix the name of the fetch global function #53227
Merged
Conversation
This file contains hidden or 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
Collaborator
|
Review requested:
|
avivkeller
approved these changes
May 31, 2024
Member
There was a problem hiding this comment.
LGTM!
Tip
I am not a core collaborator, so while this review shows my support, it doesn't have much power
Collaborator
anonrig
approved these changes
May 31, 2024
targos
approved these changes
May 31, 2024
VoltrexKeyva
approved these changes
May 31, 2024
KhafraDev
approved these changes
May 31, 2024
Collaborator
lpinca
approved these changes
May 31, 2024
Contributor
|
Related test failure: |
Contributor
|
Node.js collaborators, please do not resume CI without checking if test failures are related, otherwise the flakiness of the CI is only going to get worse. |
Member
Author
@aduh95 I pushed a fix for that to throw if the function is called, not if the property is being looked at. I preferred that over allowing the exception in the test but please let me know what you think. |
Contributor
|
@dygabo I don't think this is correct, |
Member
Author
what would be the correct behaviour then to cover this? Only test for the name of |
aduh95
reviewed
Jun 1, 2024
27008ef to
c57ac5e
Compare
aduh95
approved these changes
Jun 1, 2024
Collaborator
targos
approved these changes
Jun 1, 2024
MoLow
approved these changes
Jun 2, 2024
Collaborator
|
Landed in 6671911 |
targos
pushed a commit
that referenced
this pull request
Jun 3, 2024
PR-URL: #53227 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Merged
ebouye
pushed a commit
to ebouye/node
that referenced
this pull request
Jun 20, 2024
PR-URL: nodejs#53227 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
bmeck
pushed a commit
to bmeck/node
that referenced
this pull request
Jun 22, 2024
PR-URL: nodejs#53227 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito
pushed a commit
that referenced
this pull request
Jul 19, 2024
PR-URL: #53227 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito
pushed a commit
that referenced
this pull request
Jul 19, 2024
PR-URL: #53227 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
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.
this fixes a small regression introduced with #52275
the
global.fetch.nameshould befetchinstead ofvalue. That is also the name that the inspector gets and what is shown implicitly on the call stack during debugging. With this changeglobal.fetchwill be shown on the call stack instead ofglobal.value.Added a generic test to ensure the same applies to all the
globalfunctions.