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.

Sign up for GitHub

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

Jump to bottom

lib: make fetch sync and return a Promise #49936

Merged
merged 1 commit into from
Sep 30, 2023
Merged

lib: make fetch sync and return a Promise #49936

merged 1 commit into from
Sep 30, 2023

Conversation

KhafraDev
Copy link
Member

I haven't tracked down exactly where the webidl spec says this, but every other platform I've tested matches this behavior. This has been nagging at me for a while 😅

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Sep 28, 2023
aduh95
aduh95 approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

The title is click-baity, you didn't make fetch sync, you made it a regular function 😅

@KhafraDev
Copy link
Member Author

I honestly didn't know what to title the PR lol

@KhafraDev
Copy link
Member Author

I got this answer from the WHATWG chat:

I could be wrong but I don't think there are WebIDL interfaces that have an AsyncFunction, mostly it's regular functions that return promises, like fetch.
The AsyncFunction interface is usually for user code that's written in javascript and uses async/await internally rather than a web platform function.
Note that a "special" thing about AsyncFunction is that it can't throw, only reject promises, and it always returns a unique promise. I guess some web APIs (perhaps even fetch()) coincidentally has that guarantee without explicitly being an AsyncFunction.

@KhafraDev KhafraDev 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. labels Sep 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54344/

@KhafraDev KhafraDev removed the needs-ci PRs that need a full CI run. label Sep 29, 2023
@atlowChemi atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-bot nodejs-github-bot merged commit fa250fd into nodejs:main Sep 30, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in fa250fd

@KhafraDev KhafraDev deleted the fetch-sync-return-promise branch September 30, 2023 18:51
GeoffreyBooth pushed a commit to GeoffreyBooth/node that referenced this pull request Oct 1, 2023
@KhafraDev @GeoffreyBooth
lib: make fetch sync and return a Promise
cdaf757 
update test

PR-URL: nodejs#49936
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Oct 9, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@KhafraDev @alexfernandez
lib: make fetch sync and return a Promise
66e96f0 
update test

PR-URL: nodejs#49936
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
@KhafraDev @targos
lib: make fetch sync and return a Promise
b80e949 
update test

PR-URL: #49936
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
Merged
elrido added a commit to PrivateBin/PrivateBin that referenced this pull request Dec 3, 2023
@elrido
address nodejs unit testing issue
b150450 
mocha tests started failing as of node 20.10.0, likely due to this change:
nodejs/node#49936

Error was:
node:internal/deps/undici/undici:11730
    Error.captureStackTrace(err, this);
          ^

TypeError: Failed to parse URL from js/zlib-1.2.13.wasm
    at Object.fetch (node:internal/deps/undici/undici:11730:11)
    at async initialize (/home/runner/work/PrivateBin/PrivateBin/js/zlib-1.2.13.js:31:26) {
  [cause]: TypeError: Invalid URL: js/zlib-1.2.13.wasm
      at new URLImpl (/home/runner/work/PrivateBin/PrivateBin/js/node_modules/jsdom-url/node_modules/whatwg-url/lib/URL-impl.js:21:13)
      at new URLImplCore (/home/runner/work/PrivateBin/PrivateBin/js/node_modules/jsdom-url/lib/URLImpl.js:18:9)
      at new URLCore (/home/runner/work/PrivateBin/PrivateBin/js/node_modules/jsdom-url/lib/URL.js:28:9)
      at Object.construct (/home/runner/work/PrivateBin/PrivateBin/js/node_modules/class-proxy/index.js:18:23)
      at new Request (node:internal/deps/undici/undici:5270:25)
      at fetch (node:internal/deps/undici/undici:9508:25)
      at Object.fetch (node:internal/deps/undici/undici:11728:18)
      at fetch (node:internal/process/pre_execution:314:27)
      at initialize (/home/runner/work/PrivateBin/PrivateBin/js/zlib-1.2.13.js:31:32)
      at Object.<anonymous> (/home/runner/work/PrivateBin/PrivateBin/js/zlib-1.2.13.js:145:17)
      at Object.<anonymous> (/home/runner/work/PrivateBin/PrivateBin/js/zlib-1.2.13.js:146:4)
[...]

Notice that the error occurs on line 31, meaning that fetch is not
undefined anymore. Node works on supporting fetch, which would make our
workaround using fs.readFileSync obsolete, but it (or rather the undici
library) currently doesn't support relative URLs.
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
@KhafraDev @debadree25
lib: make fetch sync and return a Promise
f0bb380 
update test

PR-URL: nodejs#49936
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
KhafraDev added a commit to KhafraDev/node that referenced this pull request Jun 6, 2024
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
f28ef24 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
@KhafraDev KhafraDev mentioned this pull request Jun 6, 2024
Merged
KhafraDev added a commit to KhafraDev/node that referenced this pull request Jun 6, 2024
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
ae277ce 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
KhafraDev added a commit to KhafraDev/node that referenced this pull request Jun 6, 2024
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
b577692 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.
KhafraDev added a commit to KhafraDev/node that referenced this pull request Sep 25, 2024
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
9e435c2 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.
nodejs-github-bot pushed a commit that referenced this pull request Sep 26, 2024
@KhafraDev
buffer: extract Blob's .arrayBuffer() & webidl changes
da5887d 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: #49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: #53372
Refs: #49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
@KhafraDev @targos
buffer: extract Blob's .arrayBuffer() & webidl changes
3a6e724 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: #49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: #53372
Refs: #49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@KhafraDev @louwers
buffer: extract Blob's .arrayBuffer() & webidl changes
1218277 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: nodejs#53372
Refs: nodejs#49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@KhafraDev @tpoisseau
buffer: extract Blob's .arrayBuffer() & webidl changes
50f2055 
- Extracts Blob.prototype.arrayBuffer so it cannot be
  overridden in .text(), etc.
- Make .bytes() enumerable. I guess the WPT runner is
  not running the idlharness tests?
- Make .text() return a Promise, rather than being
  explicitly async. This is a non-documented part of
  the webidl spec. Refs: nodejs#49936
- Have .text(), .arrayBuffer(), and .bytes() reject
  for an invalid this instead of throwing. Fix the
  tests regarding this.

PR-URL: nodejs#53372
Refs: nodejs#49936
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

@aduh95 aduh95 aduh95 approved these changes

@JungMinu JungMinu JungMinu approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@KhafraDev @nodejs-github-bot @benjamingr @lpinca @anonrig @JungMinu @aduh95 @atlowChemi @LiviaMedeiros