search

transloadit / node-sdk

Transloadit's official Node.js SDK
https://transloadit.com
MIT License
62 stars 26 forks source link

New major version #85

Closed mifi closed 3 years ago

mifi commented 3 years ago

Proposed changes:

Inconsistencies:

Drawbacks of big rewrite:

kvz commented 3 years ago

This all sounds great. I think a new major can also make requirements on an up to date node version so we don’t have to transpile. I would be in favor of that. Ofc if we write this in typescript that goes out of the window. I’m +0 on typescript, would be in favor if you feel strongly about writing it in that.

Big +1 for jest. I don’t have strong opinions on oop/factory.

Cc’ing @ethanwillis and @tim-kos and @juliangruber for more opinions on the new major rewrite of the node sdk

juliangruber commented 3 years ago

Keep transpiling?

  • easier debugging for end users in non-transpiled code
  • complexity

I'm strongly in favor of not transpiling. What would be the advantage in keeping it?

  • Provide type definitions (or write in typescript)

Either sounds great to me :)

  • OOP API vs Factory function

Classes are the defacto standard for writing ES6 code, if anything I'd bet on the tooling to improve, or just use TS?

We can use Symbols for "more" private methods.

Inconsistencies:

  • cosmetic: camelcase vs snake_case in API e.g. template_id vs waitForCompletion

This is similar to the AWS SDK, the method names are all camelCased but the api parameters will use what the server itself expects. I think however we can be nice enough to also accept camelCased parameters and rename them in the background.

Drawbacks of big rewrite:

  • need to rewrite all tests

Is it really a full rewrite? Or is it more a question of adjusting some parameters and moving to async/await?

kvz commented 3 years ago

Another thought: I think we may have had a feature request for typescript definitions, and I think that’s a reasonable one—so we may as well write this in ts i think

mifi commented 3 years ago

I'm strongly in favor of not transpiling. What would be the advantage in keeping it?

If we want to write it in ts, then afaik we need to transpile. (Unless just providing type definitions). I’m not super familiar with TS but I can learn it.

Classes are the defacto standard for writing ES6 code, if anything I'd bet on the tooling to improve, or just use TS?

Classes are very popular yes but I’m seeing more people moving towards functions/closures and seeing the benefits of that paradigm (for example react)

This is similar to the AWS SDK, the method names are all camelCased but the api parameters will use what the server itself expects. I think however we can be nice enough to also accept camelCased parameters and rename them in the background.

Agree

Is it really a full rewrite? Or is it more a question of adjusting some parameters and moving to async/await?

Not a full rewrite, but every test would need to be changed, and if we change to jest, then I imagine it’s a full rewrite

mifi commented 3 years ago

I added a note about request being deprecated, so we might want to replace it too. If so, I vote for sindresorhus' got or axios. axios is more popular but I think axios has more hacks because it supports both browser and node.js. got is tailored for nodejs

juliangruber commented 3 years ago

Another request alternative: https://github.com/node-fetch/node-fetch. This way we build on the request library that browsers also have, Deno has, and that probably Node will some time also get. It has promise support and most of the other stuff too.

kvz commented 3 years ago

+1 for fetch from me

mifi commented 3 years ago

node-fetch is alright, although it has less built in features, like no timeout functionality and no error handling. Since got was almost a drop-in replacement for request, I already replaced it with got. But if you want node-fetch I can refactor it to use that instead, but we need to add some more custom code.

mifi commented 3 years ago

I have created a PR. Not finished yet, but API is still backward compatible with new additional async endpoints. Needs more testing but createAssembly seems to work nicely.

juliangruber commented 3 years ago

no timeout functionality and no error handling

Timeouts can be implemented using https://github.com/sindresorhus/p-timeout#readme, and I wonder how it does not have error handling? I probably don't know what you mean by that specifically.

mifi commented 3 years ago

I mean fetch doesn't have a built in timeout property that automatically handles timeout like request, got, axios etc, we need to implement it separately. fetch doesn't reject with an error in all cases (e.g. error status codes), so we need to explicitly check that, although not a big deal. Also if we ever need progress events, fetch doesn't have that built in, and file upload doesn't "just work". Fetch is more bare-bones

juliangruber commented 3 years ago

True! Since you already started with got, I'd propose to stick to that for now, and only maybe migrate to fetch if we see any problems with it?

kvz commented 3 years ago

and file upload doesn't "just work"

Please note that for uploads we'll only use tus-js-client, and got will only be used to e.g. create an Assembly and check its status (and other simple REST calls).

tim-kos commented 3 years ago

+1 for classes/oop +1 for got +1 for jest +1 async/awaitify internal code +1 promise API signature -1 for transpiling and ts

Just my two cents.

mifi commented 3 years ago

Please note that for uploads we'll only use tus-js-client, and got will only be used to e.g. create an Assembly and check its status (and other simple REST calls).

The current implementation uses TUS if it can, but it falls back to multipart form upload. It's a bit hard to see the flow in the current implementation but it can be seen here: https://github.com/transloadit/node-sdk/blob/9bb622ebbedb67fab4fc618679f3bbe2b339e592/src/TransloaditClient.js#L133 https://github.com/transloadit/node-sdk/blob/9bb622ebbedb67fab4fc618679f3bbe2b339e592/src/TransloaditClient.js#L555

kvz commented 3 years ago

Okay cool--I think for the new major, we can/should deprecate non-tus uploads

mifi commented 3 years ago

Oh, you want to print a deprecation warning if the form upload is used or just remove that code? I actually am not 100% sure when the form upload code is being used at all. It is being determined here:

https://github.com/transloadit/node-sdk/blob/9bb622ebbedb67fab4fc618679f3bbe2b339e592/src/TransloaditClient.js#L731

...so it seems like if one stream doesn't have path, e.g. is not an fs.ReadStream (a file), then it will use form upload. Doesn't TUS support non-file stream uploads? However it seems like non-file streams don't work at all (even with form), I'm gonna look into that as well (#86)

kvz commented 3 years ago

hmmm you raise an interesting point, cc-ing in @Acconut for that question

mifi commented 3 years ago

Have made a lot of progress and added more TODOs in #87 Let me know if you want to pull out some things to separate issues. I will organize it better based on which things there is already consensus to implement and which are new ideas/improvements

kvz commented 3 years ago

No need to split things up in smaller chunks afaic--unless you feel strongly or work better that way. In my mind at least, we won't let anybody use the intermediate state, just the new major which packs all of the improvements we agree on.

Regarding Needs Feedback in that PR, shall we do a call about this together with @tim-kos and @juliangruber?

Acconut commented 3 years ago

Doesn't TUS support non-file stream uploads?

tus and also tus-js-client do support uploads from non-disk based streams. However, this implies buffering in memory, so you need to be careful to not use too much RAM. The amount of buffer needed is determined by the chunkSize option in tus-js-client. Let me know if I should explain more!

mifi commented 3 years ago

Thanks. I got it to work with non-file streams, but it seems tus-js-client doesn't support streams of unknown length. Getting the error:

cannot automatically derive upload's size from input and must be specified manually using the `uploadSize` option

The developer will often not be able to know in advance the upload size of the stream. So maybe it's better to just continue using multipart form uploads for non-file streams, because it does work, and I'm thinking there's no point or too complicated/impossible trying to resume a stream that has unknown length or is not seekable?

kvz commented 3 years ago

Okay that makes sense. Unless @Acconut can poke holes in this, I do want to raise a red flag with @tim-kos 🇨🇳 because we were thinking of deprecating non-tus uploads altogether in the api. Seems we cannot do this for the streaming upload case.

Acconut commented 3 years ago

tus-js-client supports uploading streams of unknown size when the uploadLengthDeferred option is enabled (https://github.com/tus/tus-js-client/blob/master/docs/api.md#uploadlengthdeferred). This option is not activated by default because some tus servers might not support that fully (but Transloadit's servers do so).

Please try it out and let me know if it solves your issue. I will have a look and improve the error message to make it point to the uploadLengthDeferred option, so people can find it easily.

mifi commented 3 years ago

Thanks @Acconut , uploadLengthDeferred fixes the error, but I'm now having an issue with the uploaded file becoming 0 bytes (even though TUS reports success). I will look into this but I thought to let you know in case there is something obvious I'm missing.

mifi commented 3 years ago

The mighty PR is nearing completion. See "Needs feedback" under #87 - Please share your thoughts about these potential changes that may lead to more changes in the API, as well as review the breaking changes etc :)

mifi commented 3 years ago

Now released v.3.0.0-rc.1!

Acconut commented 3 years ago

uploadLengthDeferred fixes the error, but I'm now having an issue with the uploaded file becoming 0 bytes (even though TUS reports success). I will look into this but I thought to let you know in case there is something obvious I'm missing.

This is now fixed in tusd in https://github.com/tus/tusd/pull/462. Should land in production soonish.

kvz commented 3 years ago

Nice Marius!

PS i think 3 has proven itself, and we can consider: