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

feat: uploadFile support for File type #93

Merged
merged 3 commits into from
Feb 1, 2021
Merged

Conversation

agazso
Copy link
Member

@agazso agazso commented Feb 1, 2021

Fixes #90

src/index.ts Outdated Show resolved Hide resolved
Comment on lines +1 to +40
/**
* Compatibility functions for working with File API objects
*
* https://developer.mozilla.org/en-US/docs/Web/API/File
*/

export function isFile(file: unknown): file is File {
// browser
if (typeof File === 'function') {
return file instanceof File
}

// node.js
const f = file as File

return (
typeof f === 'object' &&
typeof f.name === 'string' &&
(typeof f.stream === 'function' || typeof f.arrayBuffer === 'function')
)
}

/**
* Compatibility helper for browsers where the `arrayBuffer function is
* missing from `File` objects.
*
* @param file A File object
*/
export function fileArrayBuffer(file: File): Promise<ArrayBuffer> {
if (file.arrayBuffer) {
return file.arrayBuffer()
}

// workaround for Safari where arrayBuffer is not supported on Files
return new Promise(resolve => {
const fr = new FileReader()
fr.onload = () => resolve(fr.result as ArrayBuffer)
fr.readAsArrayBuffer(file)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW if we wanted to not include this in browser bundle (which we probably in future should do to minimize bundle size) we can define file.browser.ts which exports both of those functions with much less code and in package.json in the browser field, we can swap them when the compilation target is browser.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, but I am not sure how to use it correctly.

Currently my approach is to target the browser and optimize for that because that's where the bundle size matters and the node.js version gets polyfilled. In this case both function is only necessary in the browser because node.js doesn't even implement the File Web API and I just added a workaround to isFile so that the tests pass with mocked objects.

If I move only the necessary functionality to file.browser.ts tests will fail, so eventually I will have to duplicate all the functions but then the browser ones won't be tested automatically, and even worse the compiler won't catch if you forget to duplicate a function in the .browser.ts version and it will cause a runtime error in the browser.

Do you have any idea how to solve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try and create example tomorrow. I think I know how to go about this but not sure. Feel free to merge as is.

Co-authored-by: Vojtech Simetka <vojtech@simetka.cz>
@agazso agazso merged commit 02202e9 into master Feb 1, 2021
@agazso agazso deleted the feat/upload-with-file-type branch February 1, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uploadFile should support uploading File types
2 participants