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

Update chunksDecode #2855

Closed
wants to merge 1 commit into from
Closed

Conversation

ValchanOficial
Copy link

From this bug: 2854

@ronag
Copy link
Member

ronag commented Feb 26, 2024

This looks wrong to me. But maybe someone else disagrees.

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 26, 2024

This does not make any sense. Also why would we need to call JSON.stringify all the time as JSON.stringify('') is equal to '""'.

So why do we need here two double quotes?

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Makes no sense

@ValchanOficial
Copy link
Author

ValchanOficial commented Feb 26, 2024

@ValchanOficial
Copy link
Author

ValchanOficial commented Feb 26, 2024

Line 306: resolve(JSON.parse(chunksDecode(body, length)))
chunksDecode returns ''
But JSON.parse('') doesn't work as expected

@ValchanOficial
Copy link
Author

@ValchanOficial
Copy link
Author

ValchanOficial commented Feb 26, 2024

Using return ''
image

Using return JSON.stringify('')
image

@ValchanOficial
Copy link
Author

@ronag @Uzlopak could you please take a look?

@ronag
Copy link
Member

ronag commented Feb 26, 2024

We understand what you are trying to do and we don't think it's a good idea. If we try to decode an empty string we think it should error.

@ronag ronag closed this Feb 26, 2024
@ValchanOficial
Copy link
Author

ValchanOficial commented Feb 26, 2024

We understand what you are trying to do and we don't think it's a good idea. If we try to decode an empty string we think it should error.

Shouldn't it return at least 204 then?

@ronag
Copy link
Member

ronag commented Feb 26, 2024

Who? The server? That's out of our control...

@ValchanOficial
Copy link
Author

If the person calls an external API they do not have control of the body.
Why not make the lives of those who use the lib easier?

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 26, 2024

The problem is that you are not describing the error properly.

Your bug report is actually:

An endpoint returns an empty string as payload. Fetch against this endpoint is successful. Calling body.text() returns an empty string as expected. Calling body.json() returns in an error, because an empty string can not be parsed successfully.

The question is, is throwing an error correct?
What does e.g. fetch return in chrome? Does it throw? Does it return null or undefined?
Do we have to encapsulate the JSON parse error and throw a fetch specific error?

@ValchanOficial ValchanOficial deleted the patch-1 branch February 26, 2024 20:39
@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 26, 2024

I investigated this and this is definetly working as expected.

@ValchanOficial
Copy link
Author

Do we have to encapsulate the JSON parse error and throw a fetch specific error?

Maybe just return body without parse it

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 26, 2024

No.

@ronag
Copy link
Member

ronag commented Feb 27, 2024

We follow the body mixin spec on this. Which is what our users expect.

https://developer.mozilla.org/en-US/docs/Web/API/Request/json

https://fetch.spec.whatwg.org/#ref-for-dom-body-json%E2%91%A0

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.

3 participants