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.

Sign up for GitHub

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

Jump to bottom

Allow upload strings as file content #488

Open
piranna opened this issue Oct 16, 2022 · 5 comments · May be fixed by #493
Open

Allow upload strings as file content #488

piranna opened this issue Oct 16, 2022 · 5 comments · May be fixed by #493
Labels
enhancement

Comments

@piranna
Copy link

piranna commented Oct 16, 2022

Describe the bug

At https://github.com/tus/tus-js-client/blob/master/lib/node/fileReader.js, it's being checked that the provided data is a Buffer instance or a Readable stream, but it's not possible to use a string, you need to convert it first to a Buffer yourself.

To Reproduce
Steps to reproduce the behavior:

  1. Provide a string as data in the Upload() class constructor
  2. Start upload '...'
  3. See error

Expected behavior

Allow to use strings as data input, maybe converting it internally to a Buffer if needed, but I think it can be provided directly.

Setup details
Please provide following details, if applicable to your situation:

  • Runtime environment: Node.js
  • Used tus-js-client version: 3.0.1
  • Used tus server software: tus-node-server
@piranna piranna added the bug label Oct 16, 2022
@Acconut Acconut added enhancement and removed bug labels Oct 16, 2022
@Acconut
Copy link
Member

Acconut commented Oct 16, 2022 •
edited

If I understand this correctly, you are requesting a new feature and not reporting a bug. Nowhere to my knowledge, we are advertising that you can pass strings to the Upload constructor. Or am I missing something?

To be honest, I also do not think we need to add this to tus-js-client and let users call Buffer.from on their own to convert the string into a buffer. This way they also have full control over the encoding and it does not require a lot of work from the users.

@piranna
Copy link
Author

piranna commented Oct 17, 2022

Yes, It can be seen as an improvements, and although it's true users can call Buffer.from() on their own, it's so much common to be able to use both Buffers and strings for binary data or text, that's a bit annoying to need to do It yourself, that's why I ask to add it.

@Acconut
Copy link
Member

Acconut commented Oct 18, 2022

I have never seen binary data being handled in strings, especially not inside Node.js where you have the Buffer class. But let's not discuss this here.

After thinking a bit more about this, it might also be handy for our internals. We currently have an internal test helper which constructs buffers or blob out of strings:

tus-js-client/test/spec/helpers/utils.js

Lines 8 to 17 in b031f98

/**
* Obtain a platform specific buffer object, which can be
* handled by tus-js-client.
*/
function getBlob (str) {
if (isNode) {
return Buffer.from(str)
}
return new Blob(str.split(''))
}

If we pull this functionality internally into tus-js-client we could also get rid of this function. Would you be interested in working on this?

@piranna
Copy link
Author

piranna commented Oct 18, 2022

We currently have an internal test helper which constructs buffers or blob out of strings:

Yes, my propose is to don't lead users to have to deal with this kind of hacks themselves.

If we pull this functionality internally into tus-js-client we could also get rid of this function. Would you be interested in working on this?

Sure, where can I start? Add the strings support and remove the internal getBlob() function?

@piranna piranna linked a pull request Oct 19, 2022 that will close this issue
Open
@piranna
Copy link
Author

piranna commented Oct 19, 2022

PR available at #493.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow string data on Upload constructor dyte-in/tus-js-client
2 participants
@piranna @Acconut