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

lib: fix the name of the fetch global function #53227

Merged
merged 2 commits into from
Jun 2, 2024

Conversation

dygabo
Copy link
Member

@dygabo dygabo commented May 31, 2024

this fixes a small regression introduced with #52275

the global.fetch.name should be fetch instead of value. That is also the name that the inspector gets and what is shown implicitly on the call stack during debugging. With this change global.fetch will be shown on the call stack instead of global.value.
Added a generic test to ensure the same applies to all the global functions.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup
  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 31, 2024
Copy link
Member

@avivkeller avivkeller left a comment

Choose a reason for hiding this comment

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

LGTM!

Tip

I am not a core collaborator, so while this review shows my support, it doesn't have much power

@avivkeller avivkeller added regression Issues related to regressions. fetch Issues and PRs related to the Fetch API labels May 31, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented May 31, 2024

Related test failure:

not ok 1365 parallel/test-global
  ---
  duration_ms: 110.76200
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/bootstrap/web/exposed-window-or-worker:121
                               throw new ERR_NO_CRYPTO();
                               ^
    
    Error [ERR_NO_CRYPTO]: Node.js is not compiled with OpenSSL crypto support
        at get crypto (node:internal/bootstrap/web/exposed-window-or-worker:121:34)
        at /home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-global.js:65:22
        at Array.forEach (<anonymous>)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-global.js:64:12)
        at Module._compile (node:internal/modules/cjs/loader:1460:14)
        at Module._extensions..js (node:internal/modules/cjs/loader:1544:10)
        at Module.load (node:internal/modules/cjs/loader:1275:32)
        at Module._load (node:internal/modules/cjs/loader:1091:12)
        at wrapModuleLoad (node:internal/modules/cjs/loader:212:19)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:158:5) {
      code: 'ERR_NO_CRYPTO'
    }
    
    Node.js v23.0.0-pre
  ...

@aduh95
Copy link
Contributor

aduh95 commented May 31, 2024

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.

@dygabo
Copy link
Member Author

dygabo commented Jun 1, 2024

Related test failure:

not ok 1365 parallel/test-global
  ---

@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.

@aduh95
Copy link
Contributor

aduh95 commented Jun 1, 2024

@dygabo I don't think this is correct, Object.getOwnPropertyDescriptor(globalThis, 'crypto').get.name should be the string 'get crypto', which won't be true anymore with your last commit IIUC.

@dygabo
Copy link
Member Author

dygabo commented Jun 1, 2024

@dygabo I don't think this is correct, Object.getOwnPropertyDescriptor(globalThis, 'crypto').get.name should be the string 'get crypto', which won't be true anymore with your last commit IIUC.

what would be the correct behaviour then to cover this? Only test for the name of fetch? or allow the exception in the test? Or make just the code change and don't add the test as that is new and does not seem it was needed until now?

test/parallel/test-global.js Outdated Show resolved Hide resolved
@dygabo dygabo force-pushed the fix-fetch-function-name branch from 27008ef to c57ac5e Compare June 1, 2024 08:12
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 1, 2024
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6671911 into nodejs:main Jun 2, 2024
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6671911

@dygabo dygabo deleted the fix-fetch-function-name branch June 2, 2024 19:50
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>
@RafaelGSS RafaelGSS mentioned this pull request Jun 7, 2024
sophoniie pushed a commit to sophoniie/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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fetch Issues and PRs related to the Fetch API lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.