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

the default type argument for blob.slice should be empty string #37352

Closed
jimmywarting opened this issue Feb 13, 2021 · 2 comments
Closed

the default type argument for blob.slice should be empty string #37352

jimmywarting opened this issue Feb 13, 2021 · 2 comments

Comments

@jimmywarting
Copy link

jimmywarting commented Feb 13, 2021

I took a peek at buffer.Blob.toString() and saw that the type argument was defaulted to itself

slice(start = 0, end = (this[kLength]), type = this[kType]) {

that is not the case in the browser (it's not what we are doing in fetch-blob) and the spec says it should default the type to a empty string if it's not provided

If the contentType parameter is not provided, let relativeContentType be set to the empty string.
https://w3c.github.io/FileAPI/#slice-method-algo

What is the expected behavior?

new buffer.Blob([], {type: 'text/html'}).slice().type === ''

What do you see instead?

new buffer.Blob([], {type: 'text/html'}).slice().type === 'text/html'
@jimmywarting
Copy link
Author

jimmywarting commented Feb 13, 2021

Looking at it some more i see that you validate the input arguments a bit here and there
the blob in the browser is pretty loose, where as yours is very strict and over protective

new buffer.Blob(['abc']).slice('2') // ERR_INVALID_ARG_TYPE

new window.Blob(['abc']).slice('2').text() // c
new buffer.Blob([], {type: {}}).type // ERR_INVALID_ARG_TYPE

new window.Blob([], {type: {}}).type // "[object object]"

You should just cast the type into a string whatever it is. String(type)

Just a word of caution about lowerCasing the type, should probably not do it since we loose information
w3c/FileAPI#122
So fetch-blob might do something wrong here, just best to just cast it to string without transforming i guess, unless you go the extra mile to lowercase everything but attribute values

This is quite funny:

new buffer.Blob([], { type: new String('hey') })
// TypeError [ERR_INVALID_ARG_TYPE]: The "options.type" property must be of type string.
// Received an instance of String
new buffer.Blob(['abc']).slice(new Number(2))
// TypeError [ERR_INVALID_ARG_TYPE]: The "start" argument must be of type number.
// Received an instance of Number

Edit: LoL javascript... "" instanceof String -> false

@jimmywarting
Copy link
Author

everything source in the blob constructor that is not understood should be casted into a string

await new window.Blob([ function() { return } ]).text() === "function() { return }"
await new buffer.Blob([ function() { return } ]) // [ERR_INVALID_ARG_TYPE]: The "source" argument must be of type ...

@targos targos self-assigned this Feb 13, 2021
targos added a commit to targos/node that referenced this issue Feb 13, 2021
targos added a commit to targos/node that referenced this issue Feb 13, 2021
targos added a commit to targos/node that referenced this issue Feb 13, 2021
targos added a commit to targos/node that referenced this issue Feb 16, 2021
targos added a commit to targos/node that referenced this issue Feb 17, 2021
@targos targos closed this as completed in bd4d9ef Feb 17, 2021
targos added a commit that referenced this issue Feb 28, 2021
PR-URL: #37361
Fixes: #37352
Fixes: #37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Aug 8, 2021
PR-URL: nodejs#37361
Fixes: nodejs#37352
Fixes: nodejs#37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Aug 8, 2021
PR-URL: nodejs#37361
Fixes: nodejs#37352
Fixes: nodejs#37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this issue Aug 13, 2021
PR-URL: nodejs#37361
Fixes: nodejs#37352
Fixes: nodejs#37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Aug 13, 2021
PR-URL: #37361
Backport-PR-URL: #39704
Fixes: #37352
Fixes: #37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
PR-URL: #37361
Backport-PR-URL: #39704
Fixes: #37352
Fixes: #37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
PR-URL: nodejs#37361
Backport-PR-URL: nodejs#39704
Fixes: nodejs#37352
Fixes: nodejs#37356
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos removed their assignment Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants