tus / tus-node-server

Node.js tus server, standalone or integrable in any framework, with disk, S3, Azure, and GGC stores.
https://tus.io/
MIT License
834 stars 202 forks source link

Apply Content-Type metadata consistently in all stores #656

Open Murderlon opened 2 months ago

Murderlon commented 2 months ago

Initial checklist

Problem

Ref: https://github.com/tus/tus-node-server/issues/621#issuecomment-2350769190

In the S3 store you can return contentType in the metadata of onUploadCreate to set the Content-Type but this does not work for other stores.

Solution

Make it consistent in all stores.

Alternatives

.

jdaly13 commented 2 months ago

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 -

No, content-type MUST be set to application/offset+octet-stream for PATCH requests to the tus server, as per the protocol specification. You should have the original file name in the metadata though.

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

  if (id && fileType) {
    const bucket = storage.bucket(nameOfBucket);
    const file = bucket.file(id);
    const fileMetadata = await file.setMetadata({
      contentType: fileType,
    });
    return fileMetadata;
  }

However sometimes using this bandaid causes another issue - this error from cloud storage

ApiError: The object {bucketname}/{fileId} exceeded the rate limit for object mutation operations (create, update, and delete). Please reduce your request rate. See https://cloud.google.com/storage/docs/gcs429.

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

Murderlon commented 2 months ago

Would be nice to add Remix to our examples as well in the @tus/server readme.

btw this open PR is related: #655

jdaly13 commented 2 months ago

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"

const method = request.method;
switch (method) {
    case "POST":
      return handlePostRequest(request);
    case "PATCH":
      return handlePatchRequest(request);
    case "HEAD":
      return handleHeadRequest(request);
    default:
      return new Response("Method Not Allowed", { status: 405 });
  }

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 mimeInlineBrowserWhitelist why isn't audio/mpeg included as a type?

jdaly13 commented 2 months ago

@Murderlon Just an FYI - I made a PR into Remix examples repository for Tus Resumable file uploads - please check it out https://github.com/remix-run/examples/pull/540

Murderlon commented 1 month ago

When using Tus client when does it exactly make a GET request

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.

also in the mimeInlineBrowserWhitelist why isn't audio/mpeg included as a type?

Feel free to comment this in the PR itself.

I made a PR into Remix examples repository for Tus Resumable file uploads

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.

jdaly13 commented 1 month ago

When using Tus client when does it exactly make a GET request

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.

also in the mimeInlineBrowserWhitelist why isn't audio/mpeg included as a type?

Feel free to comment this in the PR itself.

I made a PR into Remix examples repository for Tus Resumable file uploads

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

Murderlon commented 1 month ago

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.

jdaly13 commented 1 month ago

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 You'll notice there's not server file becasue it's not needed with this example.

Murderlon commented 1 month ago

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 @uppy/tus on the client, it will be practically plug-and-play.

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.

jdaly13 commented 1 month ago

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.

Murderlon commented 1 month ago

What you have is a XY problem. First of all, this repository is called tus-node-server, it's made for Node.js. Don't come with unrealistic expectations. While I made an issue to officially supported multiple runtimes, it should already be possible without too much effort. Particularly your examples, I can already deploy a Remix + tus-node-server app in Deno, Bun, AWS Lambda, and presumably workers (haven't tried that one yet).

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:

  1. Create server.js and integrate @tus/server with barebones Node.js (which most runtimes have compat for) or a framework of your choice.
  2. Create an official remix adapter
  3. Resolve https://github.com/tus/tus-node-server/issues/461 (which is more or less the same as adapters anyway)

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 @tus/server and it's stores.

jdaly13 commented 1 month ago

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.

Murderlon commented 1 month ago

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

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"

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 Response APIs.

The way forward is to work on an easy integration here, while n the meantime creating server.js is totally fair imho, and you can make it work in all/most runtimes. Pushing the burden on implementers by manually recreating @tus/server and signaling it as an "official" example within the Remix community is not the way forward I think.

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.