-
Notifications
You must be signed in to change notification settings - Fork 202
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
Apply Content-Type metadata consistently in all stores #656
Comments
Thank you @Murderlon for creating this, it is much apprecaited - I am using the GCCS Store plugin without the TUS server in a Remix JS application as I'm implementing my own logic for "POST", "PATCH", and "HEAD" requests My understanding from the other issue is -
So currently using tus implementation I'm unable to set the content-type when uploading the file, even though it's set appropriately in the metadata, my fix or band-aid currently is onSuccess handler in tusJs client I make another getRequest to my endpoint with the media type encoded in the url query string with the file ID so I can implement this code
However sometimes using this bandaid causes another issue - this error from cloud storage
so any help on this is greatly appreciated, as I'm near the finish line implementing this with Remix and hope to open source my code in the https://github.com/remix-run/examples as I was unable to implement the TusServer package however implemented each step so far |
Would be nice to add Remix to our examples as well in the @tus/server readme. btw this open PR is related: #655 |
Thanks @Murderlon not sure if I should commet here on that PR, but a couple of questions
basically in my remix JS application I have tusHandler that handles the different requests "POST" "PATCH" "HEAD"
In my handler before this switch statement I have an if statement that accepts get handler only if it has a queryString ID and filetype which I use to update metatadata after the file has been uploaded, this is manually triggered by my app after the onSuccess call in tusClient Where in the process is the get handler invoked outside of this - or is it done manually by the application meaning not part of tus client process also in the |
@Murderlon Just an FYI - I made a PR into Remix examples repository for Tus Resumable file uploads - please check it out remix-run/examples#540 |
GET is not officially part of the tus protocol. So it's never automatically initiated by the tus client, only if you want to get the file yourself, which is popular enough that it was added.
Feel free to comment this in the PR itself.
Thanks for the effort, but as I mentioned in the PR, this needs to be redone. I'll see if I can make an example. But a Next.js app router example has more priority first. |
Well your comment is disappointing and I feel it undermines the work I have done, right now Tus-Server package does not work with a remix application, even if you use the Express adapter, it is extremely tightly coupled to the request, response objects in Express. There were no examples that I could find on the web that implemented tus-server npm package with a remix application and for good reason, it was complicated to do so, My example doesn't use tus-node server npm package because it doesn't fit into remix's architecture; additionally my example works also in non-node JS environments like cloudflare workers. I was able to read through the source code and reverse engineer my implementation into a remix application. My goal with the example is to show other developers it can be done without a lot of heartache as I have done the work for them, I am fortunate that a design decision was made to make the stores "separate" npm packages because I was able to utilize those, which was helpful |
I'm sorry, didn't mean to undermine your work. However I didn't want the maintainers of remix examples to merge this and portray @tus/server this way, as I'm confident it can be done simpler. @tus/server is a barebones Node.js handler, not tightly coupled to anything specific, that can integrate into any Node.js framework. Remix will be no different. I'll take a look at creating an example next week. |
my example uses the tus protocol as an example not specific to tus-server which is specifically a Node JS handler that relies on the http req and response objects, Remix is handler that utilizes the web fetch API that can run on any JS server not specific to Node, that is why i made the example. Can you integrate tus-server into Remix JS?, probably but not without some kind of an adapter. Remix utilizes action and loader functions that take in a Request and then the application forms a Response object, to use tus-server, one needs to use an adapter then formulate some middleware, as to handle routes, tus-server relies on the handle method which takes in a req and response object, which doesn't really work with action and loader function architecture in Remix. My example gives the developer more flexibility and how to form each Response. Here is my standalone example I have pushed a public repo on my github -https://github.com/jdaly13/tus-uploads-remix |
Why of the reluctance of a server file? Unlike Next.js, it's not discouraged, in fact it's almost a best practice in the Remix ecosystem of templates. You will get tus + all the supported extensions and bug fixes while your approach is more minimal and fragile. You'll be done in just 10 lines of code. Together with Don't get me wrong, if this is the approach you want to take because you prefer for it for your own reasons, go for it. Should it be the official example? I don't think so. |
If you're using Node JS then yes you can use a server file, but you can also use my example with cloudflare workers, Deno, Bun, etc. Also like I said the fact you are using http object and extending from event emitter you would need instantiate the Server in the adapter file itself, tus-server gives you no flexibility for dynamic names of directories or bucket names. My example allows developers to do what they need in the actual route itself, tus-server doesn't give the developer the flexibility to handle content in the route itself, as the handle method expects a request and response object it's only good for a very specific basic use case and server.handle doesn't work within remix routes - once trying to use it outside that basic use case, it needs to be scrapped - basically what I've done is use the tus-protocol to give developers flexibility. |
What you have is a XY problem. First of all, this repository is called From an official example point of view (not your personal preference for your codebase), what you are doing is overly complex and unnecessary. In my opinion there are only three valid ways to go about this:
What is not a valid solution for the majority of people, is to hack together your own version tus server like you do, with all the risk involved of errors or incompatibility issues in the future. I can make no promises that your code will continue to work if I deem a change necessary between |
Imagine using a framework (Remix) and trying to use a package within that framework, but realized the package didn't really work for that framework, (there are no working examples) you spend a good amount of time implementing what that package and the protocol it's built around does and decide to implement the functionality yourself in to your application. You think to yourself, If someone runs into the same problem, I'd like to share my solution and make it available for all types of Remix users not just node users. So you take the extra time and create an example from what you done and share it with the community under Remix Examples "community" repository. You feel good about yourself as you want to help others that might run into same issue. At this point one of the maintainers of that package makes a comment on the PR to the "community" examples repo saying it shouldn't be merged and shared with the community because he feels he's the arbiter of what a "tus-protocol" example should be, because he's a maintainer of this package that doesn't provide any working examples of how it would be integrated in said framework, calling my example which I share, overcomplicated, but in actuality the sharer just tried to make it simple as possible and provided examples on how the tus-protocol works and how basically it uses 3 different request methods, the maintainer of said project criticizes but provides no solution. Honestly I feel you're just being rude; the PR you mentioned above is over a year old and still open and no solution and you still haven't provided an example of how to integrate this package into Remix, if you were to provide a PR request into a Remix examples then great, there could be two examples and users could decide which one they want to use, again it's a community repository. I'm not going to comment any further on this. So if you want you can have the last word, I'm literally just trying to help others. |
Alright I think we both had enough of this discussion. To conclude, open source is neither a community nor a democracy, no one not entitled to anything. When you say
remember that this is completely free. You didn't pay me for support or to fix something. Regarding the example, I actually try to put a lot of care into my documentation and I'd like to think it's already decently example-driven, as I know that's what people scan for. Granted, I didn't do Remix (yet). I actually spend a lot of time maintaining this package so I think I reserve the right to provide my opinion in an another "community" repository if it's about the package I maintain, especially if first time viewers of this package see the example and our daunted by the integration code. I already provided the ways we can go forward from here. If you want to be truly helpful, you can post your findings in #461 on how to best approach making adapters from Node.js to The way forward is to work on an easy integration here, while n the meantime creating Lastly, I didn't mean to disregard your good intentions or effort. I just think if we worked out the best way forward here, then there wouldn't have been wasted effort. |
Initial checklist
Problem
Ref: #621 (comment)
In the S3 store you can return
contentType
in themetadata
ofonUploadCreate
to set the Content-Type but this does not work for other stores.Solution
Make it consistent in all stores.
Alternatives
.
The text was updated successfully, but these errors were encountered: