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

Silent behavior on no body responses #37

Open
insyri opened this issue Mar 8, 2023 · 1 comment
Open

Silent behavior on no body responses #37

insyri opened this issue Mar 8, 2023 · 1 comment
Assignees
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@insyri
Copy link
Owner

insyri commented Mar 8, 2023

Abstract

The httpRaw method (concerned portion shown below) has a defect where it will resolve the function normally if there is no body. This skips type checks since the empty response it is casted to the generic type of the expected structure and thus lies to users. Without knowing the internal functionality of how it resolves this case, users may find unexpected bugs that can skip this check. Of course, this method isn't exactly expected to be adopting use in projects depending on Tpy, however, since the method is made public, it should be able to be used as normally.

tpy/src/tpy.ts

Lines 350 to 354 in 6c9a2ca

const body = await response.text();
if (response.ok) {
// This is bad, I know. You fix it. Please.
return (body.length === 0 ? {} : JSON.parse(body)) as unknown as T;
}

This original functionality was created with the KV put operations in mind (see put and putArrayBuffer; notice the methods do not capture the response) since those endpoints return no body. If I remember correctly, the reason behind this design choice was to avoid some type collisions internally, mostly a lazy -or uneducated- solution.

Proposal

I imagine those who are accessing endpoints that return no body expect this behavior, this leads me to believe that the proper approach to this would be to tell the type system to expect this, negating the generic type (if there is one) entirely and returning nothing at all or some nullish value.

Since we would also be changing the logic, this would go beyond the type system and implemented into the method itself. This may entail an extra parameter and maybe a type parameter (or at least a conditional operation) to follow this process of logic.

@insyri insyri added bug Something isn't working invalid This doesn't seem right labels Mar 8, 2023
@insyri insyri self-assigned this Mar 8, 2023
@insyri
Copy link
Owner Author

insyri commented Mar 9, 2023

Something I ran into while creating this parameter: as this kind of parameter would have inserted data in certain situations, it cannot be appended to the end of the list of the parameters of httpRaw. It would be required for the arrangement of the parameters to be changed. This would require a change in all invocations, yes, however, my concern is more that these parameters are becoming more complex making it hard to remember or predict what the next parameter will be. I think it may be beneficial to change additional options (optional parameters) to an entire object following an interface.

This also reminds me of the current constructor parameter setup. Should this be addressed in a later issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant