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

Pass response to onSuccess callback #689

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

netdown
Copy link

@netdown netdown commented May 14, 2024

Support using customized responses as introduced in tus/tus-node-server#615

@Acconut
Copy link
Member

Acconut commented May 14, 2024

Thank you for this PR, but I am concerned that this is too confusing for users. The response that is passed to onSuccess this way might be a POST, PATCH, or HEAD response depending on when the upload is finished. If users expect the value in onSuccess to always be the last PATCH request, then they will run into issues. Instead we commend people to use the onAfterResponse callback, which provides access to all responses and also makes it clear that there might be multiple requests involved in the upload process. Does this not work for you?

@netdown
Copy link
Author

netdown commented May 14, 2024

Indeed I wasn't cautious enough. What if the argument is only passed for post and patch requests' events? Or maybe like { requestMethod: 'PATCH'|'POST'|'HEAD', response: HttpResponse };

My goal is being able to access response data in Uppy's complete event using uppy.getFiles(). Unfortunately neither headers nor status code (actually the response object contains a status code, but the value is hard coded to 200) are returned using tus, so I know nothing but the upload url.

@Acconut
Copy link
Member

Acconut commented May 14, 2024

Have you tried the onAfterResponse callback? Did that not work for you? It should also be usable from Uppy.

@netdown
Copy link
Author

netdown commented May 15, 2024

It feels really hacky that I'd need to decide which response is a success response and store them in a custom state, then at the complete handler, make sure my state is actually in sync with uppy's state, find the appropriate response for each file, and make sure uppy processing haven't failed for the responses I assumed to be ok. Seems too complicated and prone to bugs while it could be solved by passing the response. Are you sure the object signature I presented is not suitable?

@Acconut
Copy link
Member

Acconut commented May 15, 2024

Are you sure the object signature I presented is not suitable?

No, I am not very sure and cannot guarantee that my opinion doesn't change :) I am trying to considering the bigger picture here and how we can improve the overall approach. The problem is that in tus, there is no standardized way of sending information from the server back to the client as tus focuses on uploads where the client sends data to the server. This has lead to the current situation where additional data from the server is added to PATCH responses for clients to retrieve. But this is not ideal because the information in these responses cannot be re-retrieved if the clients needs to fetch it again. So overall, I would like to have a more holistic approach for sending data from the server, which solves this problem in a proper way without shortcomings. Some discussion on this has happened in tus/tus-resumable-upload-protocol#190.

Therefore, I would like to reserve a future argument for onSuccess to be whatever approach we come up with there instead of using it right now for the last response right now, which might not make sense in the future. Is that understandable?

@netdown
Copy link
Author

netdown commented May 15, 2024

I regret to hear. I briefly checked the discussion, and I would like to point out that IMO - and if I'm not mistaken - forcing Content-Location raises a pointless obstacle in most use cases. Storing the tus upload id and creating a separate endpoint just to be able to pass the final id to the client does not make sense. I don't care about tus id nor I want to let the client requery anything, I just want to know the id returned from my server, which seems to be the most common use case.

Additionally, I'm not sure requerying some data about the uploaded file is not out of tus territory. The upload has been completed, why bother post-upload logic? Just make the headers and body available to the client, so multiple approaches can be implemented.

@Acconut
Copy link
Member

Acconut commented Jun 1, 2024

I can understand that an extra round trip for fetching the Content-Location resource is not desired in your case. You will always have the ability to add custom headers to responses and retrieve them on the client side to implement custom logic.

Additionally, I'm not sure requerying some data about the uploaded file is not out of tus territory. The upload has been completed, why bother post-upload logic?

That's definitely a question worth asking and my opinioned in the past year has developed to be that tus implementations should assist you with postprocessing in some way as this makes file handling in applications more straightforward, robust and seamsless. Of course, that doesn't mean that we have to offer a full solution for postprocessing files. But these questions come up very often and I would prefer if we can offer some answer. Using them is then up to each person on their own.

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.

None yet

2 participants