Skip to content

fix: avoid reading files when the size is empty #134

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 10, 2022
Merged

fix: avoid reading files when the size is empty #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9517b0d
fix: avoid reading files when the size is empty
jimmywarting Mar 10, 2022
68f88e2
also use stat.size for checking if it is modified
jimmywarting Mar 10, 2022
dfe7c96
bump minor version number
jimmywarting Mar 10, 2022
568b309
Don't push empty chunks into the array (creates faster reads)
jimmywarting Mar 10, 2022
d4632ad
reverted back to avoid making a if check
jimmywarting Mar 10, 2022
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
10 changes: 8 additions & 2 deletions from.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ class BlobDataItem {
this.#start = options.start
this.size = options.size
this.lastModified = options.lastModified
this.originalSize = options.originalSize === undefined
Copy link
Member

Choose a reason for hiding this comment

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

I personally try to treat null and undefined the same in the code, to avoid unexpected behaviour. This also lets us transition to the ?? operator when we drop support for Node.js 12.x, without changing the behaviour of the code.

Suggested change
this.originalSize = options.originalSize === undefined
this.originalSize = options.originalSize == null

(x == null is the same as x === undefined || x === null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i know that, i also try to avoid null from time to times. and i can't wait until we can start using ??=

but i wanted to avoid using == instead of === and breaking any compatibility with nullish stuff
also since there is no occurrences of null in the code, then i tought it would be fine to just check for undefined

could also just do this.originalSize = options.originalSize || options.size

Copy link
Member

Choose a reason for hiding this comment

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

options.originalSize || options.size will use options.size if options.originalSize is 0 though, that's probably not what we want

Copy link
Contributor Author

@jimmywarting jimmywarting Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i thought about it too, but after commit 568b309 then that might not be needed after all, cuz 0-sized blob and typedarrays will not be in the source anymore

? options.size
: options.originalSize
}

/**
Expand All @@ -75,16 +78,19 @@ class BlobDataItem {
return new BlobDataItem({
path: this.#path,
lastModified: this.lastModified,
originalSize: this.originalSize,
size: end - start,
start: this.#start + start
})
}

async * stream () {
const { mtimeMs } = await stat(this.#path)
if (mtimeMs > this.lastModified) {
const { mtimeMs, size } = await stat(this.#path)

if (mtimeMs > this.lastModified || this.originalSize !== size) {
throw new DOMException('The requested file could not be read, typically due to permission problems that have occurred after a reference to a file was acquired.', 'NotReadableError')
}

yield * createReadStream(this.#path, {
start: this.#start,
end: this.#start + this.size - 1
Expand Down
8 changes: 6 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,12 @@ const _Blob = class Blob {
part = encoder.encode(`${element}`)
}

this.#size += ArrayBuffer.isView(part) ? part.byteLength : part.size
this.#parts.push(part)
const size = ArrayBuffer.isView(part) ? part.byteLength : part.size
// Avoid pushing empty parts into the array to better GC them
if (size) {
this.#size += size
this.#parts.push(part)
}
}

this.#endings = `${options.endings === undefined ? 'transparent' : options.endings}`
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "fetch-blob",
"version": "3.1.4",
"version": "3.1.5",
"description": "Blob & File implementation in Node.js, originally from node-fetch.",
"main": "index.js",
"type": "module",
Expand Down
6 changes: 6 additions & 0 deletions test/own-misc-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@ promise_test(async () => {
assert_equals(await (await fileFrom('./LICENSE')).text(), license.toString())
}, 'blob part backed up by filesystem slice correctly')

promise_test(async () => {
fs.writeFileSync('temp', '')
await blobFromSync('./temp').text()
fs.unlinkSync('./temp')
}, 'can read empty files')

test(async () => {
const blob = blobFromSync('./LICENSE')
await new Promise(resolve => setTimeout(resolve, 2000))
Expand Down