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

Body Mixin text() is supposed to return USVString #39804

Closed
ronag opened this issue Aug 18, 2021 · 6 comments
Closed

Body Mixin text() is supposed to return USVString #39804

ronag opened this issue Aug 18, 2021 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. web streams

Comments

@ronag
Copy link
Member

ronag commented Aug 18, 2021

https://fetch.spec.whatwg.org/#body-mixin

@jasnell I think the Blob and consumer helper text() methods need to run through toUSVString which is used in the url module?
@ronag ronag changed the title Body Mixin Text is supposed to return USVString Body Mixin text() is supposed to return USVString Aug 18, 2021
@jasnell
Copy link
Member

jasnell commented Aug 18, 2021

To be fully compliant, yes we likely need to.

@PurnashisHazra
Copy link

Should I take this issue up?

@ronag ronag added good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. web streams labels Aug 24, 2021
@jimmywarting
Copy link

jimmywarting commented Aug 27, 2021 •
edited
Loading

just notice util.toUSVString came out in 16.8, good for ppl writing polyfills... got a side question: dose TextDecoder.decode() return a USVString? cuz that is what i have been using in some .text() methods.... This will also remove BOM

I also wonder what the diffrent is between USVString and a "normal" string is and doing String(val) or ${val} to convert them
Maybe stream-consumers text(iterable) should resolve with a USVString also?

@WenheLI
Copy link

WenheLI commented Sep 1, 2021

Anyone currently working on this issue? If not, can I try to solve it?

@git-srinivas
Copy link
Contributor

git-srinivas commented Oct 3, 2021 •
edited
Loading

Hi @ronag I am working on this issue. I could see the Blob contains the text() method. what is consumer helper here? can you please point the file or provide some more information about it?

@Mesteery
Copy link
Contributor

Mesteery commented Oct 4, 2021

https://github.com/nodejs/node/blob/master/lib/stream/consumers.js

git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 6, 2021
@git-srinivas
lib: body mixin text is to return usvstring
0a2fcb8 
Readable web stream returns  strings.
Where it supposed to return usvstring. This change will fix the issue

Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 6, 2021
@git-srinivas
lib: added test case for consumers.js and blob.js
98dd270 
Added test cases for the code changes.

Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 6, 2021
@git-srinivas
lib: lint errors fixed in consumers.js
0995716 
Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 7, 2021
@git-srinivas
lib: lint errors fixed in consumers.js
18827d1 
Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
@git-srinivas git-srinivas mentioned this issue Oct 7, 2021
Closed
2 tasks
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 13, 2021
@git-srinivas
lib: body mixin text is to return usvstring
c39de70 
Readable web stream returns  strings.
Where it supposed to return usvstring. This change will fix the issue

Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 13, 2021
@git-srinivas
lib: added test case for consumers.js and blob.js
bbe2a0e 
Added test cases for the code changes.

Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 13, 2021
@git-srinivas
lib: lint errors fixed in consumers.js
442f969 
Fixes: nodejs#39804
Refs: https://fetch.spec.whatwg.org/#body-mixin
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 13, 2021
@git-srinivas
test: refactor testcases for surrogate codes
5fa492a 
Since the readable web streams  already returns
USVString
The changes made to blob.js and consumers.js  are reverted
Test cases are added to check  surrogate to USVString conversion for
ReadablewebStreams Textdecoder and Blob

PR-URL: nodejs#40351
Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 13, 2021
@git-srinivas
test: lint errors fixed in test cases
37ab7b0 
PR-URL: nodejs#40351
Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 21, 2021
@git-srinivas
test: test cases for surrogate to usv string
4fb8c8a 
Added test cases to check surrogate code points to usv string conversion
in stream consumers and text decoder

Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 21, 2021
@git-srinivas
test: test cases for surrogate to usv string
e5857ba 
Added test cases to check surrogate code points to usv string conversion
in stream consumers and text decoder

Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 22, 2021
@git-srinivas
test: incomplete utf-8 bytes to usv string
22de91f 
Added test cases for below scenarios
-text() method of stream consumers.js
and blob.js should  return usv string for incomplete utf-8 byte sequnce.
-the textdecoder should  return usv string for
incomplete utf-8 byte sequnce.

Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 22, 2021
@git-srinivas
test: incomplete utf-8 bytes to usv string
50b943b 
Added test cases for below scenarios
-text() method of stream consumers.js
and blob.js should  return usv string for incomplete utf-8 byte sequnce.
-the textdecoder should  return usv string for
incomplete utf-8 byte sequnce.

Fixes: nodejs#39804
git-srinivas added a commit to git-srinivas/node that referenced this issue Oct 23, 2021
@git-srinivas
test: add tests for invalid UTF-8
75c69af 
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.

Fixes: nodejs#39804
@ronag ronag closed this as completed Oct 28, 2021
lpinca pushed a commit that referenced this issue Nov 15, 2021
@git-srinivas @lpinca
test: add tests for invalid UTF-8
f1aa85d 
Verify that `Blob.prototype.text()`, `streamConsumers.text()` and
`TextDecoder.prototype.decode()` work as expected with invalid UTF-8.

PR-URL: #40351
Fixes: #39804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: add tests for invalid UTF-8 git-srinivas/node
7 participants
@jasnell @jimmywarting @ronag @git-srinivas @WenheLI @PurnashisHazra @Mesteery