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

fix(upload): return type on POST is now string #976

Merged
merged 7 commits into from
Apr 15, 2024

Conversation

lyager
Copy link
Contributor

@lyager lyager commented Feb 21, 2024

A POST to a webserver can not always be expected to be a JSON response.

Success is now determined by the HTTP return code.

Upon success the body content is returned as a string.

@lyager lyager requested a review from a team as a code owner February 21, 2024 13:55
@lyager
Copy link
Contributor Author

lyager commented Feb 21, 2024

This might be more of a suggestion - not all web services receiving a post returns JSON. Propose to leave result processing to the end-user

@FabianLars
Copy link
Member

Hmm, you're right with that, however we actually ignore the return value on the js side so we may as well return Result<()> from the command instead so we can get rid of both res.json() and res.text() and really only use the status.

For that status error i'd prefer if you could add a new Error enum variant instead of using ContentLength

Lastly, it would be much appreciated if you could apply the same to the download command after making those changes :)

P.S. Thank you for contributing!

@lyager
Copy link
Contributor Author

lyager commented Feb 29, 2024

Thanks for the feedback @FabianLars I somehow missed your response. I'll give it another shot hopefully soon

@lyager
Copy link
Contributor Author

lyager commented Feb 29, 2024

@FabianLars Ok, I think that was all of your suggestions :) What do you think?

@lyager
Copy link
Contributor Author

lyager commented Mar 4, 2024

.. my default linter formater apperantly is different that cargo fmt - I fixed it. Sorry.

@FabianLars
Copy link
Member

There's still this part open from above:

Hmm, you're right with that, however we actually ignore the return value on the js side so we may as well return Result<()> from the command instead so we can get rid of both res.json() and res.text() and really only use the status.

So if there's any value in the response body we'll also have to fix the js side to actually forward it to the user.

@lyager
Copy link
Contributor Author

lyager commented Mar 14, 2024

@FabianLars You're right. It should be fixed now.

I also added @lucasfernog suggestion.

I'll do some extensive test, but I think this should finally be it 😅

Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

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

We also need a change file (a markdown file under the .changes folder) like this:

---
"upload": patch
"upload-js": patch
---

Return the upload response as a string and error out if the status code is not within 200-299.

plugins/upload/src/lib.rs Outdated Show resolved Hide resolved
plugins/upload/src/lib.rs Outdated Show resolved Hide resolved
@lucasfernog
Copy link
Member

Tried to push changes to your repo but i'm not allowed to :(

@lyager
Copy link
Contributor Author

lyager commented Mar 21, 2024

@lucasfernog Thank you for your help, there are clearly some corners in the repo that are unknown to me. I've accepted you proposals in the Github UI.

@lucasfernog
Copy link
Member

@lyager please also push the change file i mentioned above :)

@lyager
Copy link
Contributor Author

lyager commented Mar 25, 2024

Thanks for the patience @lucasfernog , I've rebased and added the covector file you mentioned.

@lucasfernog
Copy link
Member

idk why but the diff is crazy right now :( something went wrong with your rebase

lyager and others added 7 commits April 2, 2024 08:42
A POST to a webserver can not always be expected to be a JSON response.

Success is now determined by the HTTP return code.

Upon success the body content is returned as a string.
Not all embedded devices are acceptable to receiving unspecified amounts
of data. Appending the content-length up front helps this devices
succeed.
The return values was not used.

On POST the HTTP error code is returned as an enum.
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
@lyager
Copy link
Contributor Author

lyager commented Apr 2, 2024

@lucasfernog I've rebased against v1, now only 3 files have changed.. Sorry about that, and thank you for your patience.

@lyager lyager requested a review from lucasfernog April 11, 2024 07:14
@FabianLars FabianLars dismissed lucasfernog’s stale review April 15, 2024 13:15

ain't nobody got time for that (i'm merging the last v1 prs. we can iterate over them again when we merge v1 into v2)

@FabianLars FabianLars merged commit 4a5ab18 into tauri-apps:v1 Apr 15, 2024
8 checks passed
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