FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: make fetch sync and return a Promise by KhafraDev · Pull Request #49936 · nodejs/node · GitHub

/ node Public

lib: make fetch sync and return a Promise - #49936

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
KhafraDev:fetch-sync-return-promise
Sep 30, 2023
ConversationCommits (1)ChecksFiles changed
Merged

lib: make fetch sync and return a Promise#49936
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
KhafraDev:fetch-sync-return-promise

Conversation

KhafraDev commented Sep 28, 2023

Copy link
Copy Markdown
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 added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Sep 28, 2023
aduh95 reviewed Sep 28, 2023
View reviewed changes
Comment thread test/parallel/test-fetch.mjs Outdated
aduh95 approved these changes Sep 28, 2023
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

KhafraDev force-pushed the fetch-sync-return-promise branch from c06b18c to e79c73c Compare September 28, 2023 20:30

KhafraDev commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

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

KhafraDev commented Sep 28, 2023

Copy link
Copy Markdown
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 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 28, 2023
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 commented Sep 28, 2023

Copy link
Copy Markdown
Collaborator

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

KhafraDev removed the needs-ci PRs that need a full CI run. label Sep 29, 2023
atlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
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 merged commit fa250fd into nodejs:main Sep 30, 2023

nodejs-github-bot commented Sep 30, 2023

Copy link
Copy Markdown
Collaborator

Landed in fa250fd

KhafraDev deleted the fetch-sync-return-promise branch September 30, 2023 18:51
RafaelGSS mentioned this pull request Oct 9, 2023
Merged
targos pushed a commit that referenced this pull request Nov 11, 2023
lib: make fetch sync and return a Promise
b80e949 
update test

PR-URL: #49936
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos mentioned this pull request Nov 12, 2023
Merged
elrido added a commit to PrivateBin/PrivateBin that referenced this pull request Dec 3, 2023
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 mentioned this pull request Dec 6, 2023
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
lib: make fetch sync and return a Promise
f0bb380 
update test

PR-URL: nodejs#49936
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
KhafraDev mentioned this pull request Jun 6, 2024
Merged
KhafraDev added a commit to KhafraDev/node that referenced this pull request Sep 25, 2024
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
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 <yagiz@nizipli.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
targos pushed a commit that referenced this pull request Oct 4, 2024
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 <yagiz@nizipli.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
anonrig anonrig approved these changes
JungMinu JungMinu approved these changes
aduh95 aduh95 approved these changes
atlowChemi atlowChemi approved these changes
LiviaMedeiros LiviaMedeiros approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL