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

HTTP server fails if body is a String object #1269

Closed
tve opened this issue Dec 14, 2023 · 10 comments
Closed

HTTP server fails if body is a String object #1269

tve opened this issue Dec 14, 2023 · 10 comments

Comments

@tve
Copy link
Contributor

tve commented Dec 14, 2023

Moddable SDK version: 4.3
Target device: esp32

Description
When a "prepareResponse" callback of HTTP server returns a body consisting of a String object (as opposed to a string) the server throws an exception causing the request to be aborted. The reason for this is a check against typeof this.body:

count = ("string" === typeof this.body) ? this.body.length : this.body.byteLength;	//@@ utf-8 hell

https://github.com/Moddable-OpenSource/moddable/blob/public/modules/network/http/http.js#L687

Steps to Reproduce

  1. Create a handler that returns something like {status: 200, body: new String('foo')}

Note that this is not the only instance of this erroneous check in this module...

@phoddie
Copy link
Collaborator

phoddie commented Dec 14, 2023

I'm not sure this is a bug. The API accepts a string or an object. If it is an object, it needs to be a buffer/view.

What would you propose to change here?

@tve
Copy link
Contributor Author

tve commented Dec 14, 2023

The docs state "If the body is a String or ArrayBuffer, it is the complete response." I am literally returning a String. Also, I believe it is fair to say that generally JS developers view string and String to behave the same (e.g. note the use of the String class in headersComplete: I assume the callback gets a string not a String...). If you do not want to support String then I suggest fixing the docs to say "string (not a String object)".

@tve
Copy link
Contributor Author

tve commented Dec 14, 2023

NB: in light of #1271 maybe the best is to either not support strings or to consistently perform a TextEncoder transformation right away.

@phoddie
Copy link
Collaborator

phoddie commented Dec 14, 2023

I understand it is subtle, but there is a difference between a string and a boxed string (string object). When they behave the same, it is generally because the implementation cals toString() on the argument. That's not an option here because an object is interpreted as a buffer. I don't see a strong need for a code change. The caller can easily pass a string instead of a string object.

The ECMA-419 socket and network protocol implementations (e.g. http) don't accept strings, only buffers. That avoids the problem.

@tve
Copy link
Contributor Author

tve commented Dec 14, 2023

I guess it comes down to overall developer experience. If the docs had been accurate it would have saved me a bunch of time and aggravation. If the code had lived up to the docs, ditto. I know the difference between String and string but it's not something the vast majority of JS APIs slap me in the face with.

@phoddie
Copy link
Collaborator

phoddie commented Dec 14, 2023

I think this might be a simple solution. Around line 678, change:

	this.body = response.body;

...to...

	this.body = (response.body instanceof String) ? response.body.valueOf() : response.body;

@phoddie
Copy link
Collaborator

phoddie commented Dec 14, 2023

it's not something the vast majority of JS APIs slap me in the face with.

This API accepts a string or an object. In that case, the distinction matters. I'm sorry you don't like it, but it is what it is here.

@tve
Copy link
Contributor Author

tve commented Dec 15, 2023

I can confirm that the code change fixes the issue for me.

@phoddie
Copy link
Collaborator

phoddie commented Dec 15, 2023

Thanks. I'll commit that.

@phoddie
Copy link
Collaborator

phoddie commented Dec 23, 2023

Closing (fixed).

@phoddie phoddie closed this as completed Dec 23, 2023
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

No branches or pull requests

2 participants