Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants