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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5e02da
fix: support @web-std/file in normalize input
vasco-santos Jul 20, 2021
ded6c36
chore: validate blob/file have a stream property
vasco-santos Jul 20, 2021
5f798ef
chore: fallback to web-std blob
vasco-santos Jul 20, 2021
5e8c992
fix: tests
vasco-santos Jul 20, 2021
d0c30ce
chore: use latest file version
vasco-santos Jul 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/ipfs-core-utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"uint8arrays": "^2.1.6"
},
"devDependencies": {
"@web-std/file": "^1.1.0",
"aegir": "^34.0.2",
"rimraf": "^3.0.2"
}
Expand Down
4 changes: 1 addition & 3 deletions packages/ipfs-core-utils/src/files/normalise-input/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
'use strict'

const { Blob } = globalThis

/**
* @param {any} obj
* @returns {obj is ArrayBufferView|ArrayBuffer}
Expand All @@ -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

*/
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

}

/**
Expand Down
16 changes: 15 additions & 1 deletion packages/ipfs-core-utils/test/files/normalise-input.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { expect } = require('aegir/utils/chai')
const blobToIt = require('blob-to-it')
const uint8ArrayFromString = require('uint8arrays/from-string')
const all = require('it-all')
const { File } = require('@web-std/file')
const { Blob, ReadableStream } = globalThis
const { isBrowser, isWebWorker, isElectronRenderer } = require('ipfs-utils/src/env')

Expand Down Expand Up @@ -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 😡

expect(content).to.be.an.instanceOf(Blob)
} catch (err) {
// Fallback to instance of File
expect(content).to.be.an.instanceOf(File)
}
content = blobToIt(content)
}

Expand Down Expand Up @@ -173,6 +179,14 @@ describe('normalise-input', function () {
testInputType(BLOB, 'Blob', false)
})

describe('@web-std/file', () => {
it('normalizes File input', async () => {
const FILE = new File([BUFFER()], 'test-file.txt')

await testContent(FILE)
})
})

describe('Iterable<Number>', () => {
testInputType(ARRAY, 'Iterable<Number>', false)
})
Expand Down