Skip to content

fix(fetch): toUSVString#986

Merged
ronag merged 4 commits intomainfrom
usvstring
Aug 20, 2021
Merged

fix(fetch): toUSVString#986
ronag merged 4 commits intomainfrom
usvstring

Conversation

@ronag
Copy link
Copy Markdown
Member

@ronag ronag commented Aug 18, 2021

No description provided.

@ronag ronag requested a review from mcollina August 18, 2021 20:15
Comment thread lib/fetch/util.js Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 18, 2021

Codecov Report

❌ Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.88%. Comparing base (b7ee4b3) to head (1169e94).
⚠️ Report is 2366 commits behind head on main.

Files with missing lines Patch % Lines
lib/fetch/formdata.js 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
+ Coverage   90.80%   93.88%   +3.08%     
==========================================
  Files          37       37              
  Lines        3587     3614      +27     
==========================================
+ Hits         3257     3393     +136     
+ Misses        330      221     -109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina
Copy link
Copy Markdown
Member

I don't understand what this brings.

@ronag
Copy link
Copy Markdown
Member Author

ronag commented Aug 19, 2021

I don't understand what this brings.

Placeholder for spec compliance. The spec says that these should be "usvstring"s. We need to eventually either implement _toUSVString ourselves or have node core expose it.

@ronag
Copy link
Copy Markdown
Member Author

ronag commented Aug 19, 2021

We do the same thing for the URL module in node core.

@mcollina
Copy link
Copy Markdown
Member

Awesome, ok. Let's expose this from core.

ronag added a commit to nxtedition/node that referenced this pull request Aug 19, 2021
Expose toUSVString to it can be used by user libraries.

Refs: nodejs/undici#986
ronag added a commit to nxtedition/node that referenced this pull request Aug 19, 2021
Expose toUSVString to it can be used by user libraries.

Refs: nodejs/undici#986
@ronag ronag merged commit 31de23e into main Aug 20, 2021
lpinca pushed a commit to nodejs/node that referenced this pull request Aug 22, 2021
Expose toUSVString to it can be used by user libraries.

PR-URL: #39814
Refs: nodejs/undici#986
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
lpinca pushed a commit to nodejs/node that referenced this pull request Aug 22, 2021
Expose toUSVString so it can be used by user libraries.

PR-URL: #39814
Refs: nodejs/undici#986
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Aug 22, 2021
Expose toUSVString so it can be used by user libraries.

PR-URL: #39814
Refs: nodejs/undici#986
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Sep 4, 2021
Expose toUSVString so it can be used by user libraries.

PR-URL: #39814
Refs: nodejs/undici#986
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ronag ronag deleted the usvstring branch October 28, 2021 08:10
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* fix(fetch): toUSVString

* fixup

* fixup

* fixup
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix(fetch): toUSVString

* fixup

* fixup

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants