Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: support @web-std/file in normalize input #3750

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

vasco-santos
Copy link
Member

@@ -15,7 +13,7 @@ function isBytes (obj) {
* @returns {obj is Blob}
*/
function isBlob (obj) {
return typeof Blob !== 'undefined' && obj instanceof Blob
return obj.constructor && (obj.constructor.name === 'Blob' || obj.constructor.name === 'File')
Copy link
Member

Choose a reason for hiding this comment

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

How does this prevent an instance of any old class named "File" or "Blob" being detected as a File or Blob?

Maybe ensure that methods are present on obj that we will call later on?

Copy link
Member Author

Choose a reason for hiding this comment

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

this sounds a good idea

@@ -15,7 +13,7 @@ function isBytes (obj) {
* @returns {obj is Blob}
Copy link
Member

Choose a reason for hiding this comment

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

Because the const { Blob } = ... line was removed, this is now the global Blob so TS will be unhappy if someone explicitly passes @web-std/file objects in your code.

Might just need to remove the @returns annotation so tsc will derive a boolean return type.

Copy link
Member Author

Choose a reason for hiding this comment

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

return the @returns causes typescript to also not be happy, I added globalThis to the type

@@ -35,7 +36,12 @@ async function verifyNormalisation (input) {
let content = input[0].content

if (isBrowser || isWebWorker || isElectronRenderer) {
expect(content).to.be.an.instanceOf(Blob)
try {
Copy link
Member

Choose a reason for hiding this comment

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

File extends Blob so under what conditions would we need to fall back here?

Copy link
Member Author

Choose a reason for hiding this comment

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

instanceOf is tricky.
File https://github.com/web-std/file/blob/default/src/lib.js#L6 extends from Blob as you mention, and Blob is https://github.com/web-std/io/blob/main/blob/src/lib.js (so the same as here).

But, somehow this will not work with instanceOf Blob, and works with @web-std/blob. So, if you create a Blob and a @web-std/blob, their instanceOf are not compatible...

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like @web-std/blob needs to return the native implementation if it's available?

Copy link
Member

Choose a reason for hiding this comment

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

Also of note here is that node 16 has a Blob implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This sounds like @web-std/blob needs to return the native implementation if it's available?

It does: https://github.com/web-std/io/blob/main/blob/src/lib.js

but instanceOf returns false 🤷🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it now to Blob to be:

const { Blob } = require('@web-std/blob')

instead of

const { Blob } = globalThis

It works this way, but then other tests fail (which probably use the native Blob 🤷🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

reverting this, as this is even more problematic

Copy link
Member Author

Choose a reason for hiding this comment

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

So, when creating a https://github.com/web-std/io/tree/main/file in the browser, it will not be an instance of Blob, but it will be of https://github.com/web-std/io/tree/main/blob. This is really weird, given that ws Blob is really globalThis.Blob.

This codepen shows that a browser File should be instance of both Blob and ws Blob.

My only assumption for this to not be the case in the tests here, is that somehow playwright has a different globalThis scope in the test that makes them being different... @hugomrdias is this reasonable? do you have any clues?

Copy link
Member Author

Choose a reason for hiding this comment

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

web-std/io#10 this fixed the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Now @web-std/file uses the Node.js version with Eletron renderer 😡

@vasco-santos vasco-santos force-pushed the fix/support-web-std-file-in-normalize-input branch from feb1a2f to d09c419 Compare July 20, 2021 13:46
@vasco-santos vasco-santos force-pushed the fix/support-web-std-file-in-normalize-input branch from d09c419 to 5f798ef Compare July 20, 2021 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants