-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
fix: Blob constructor on various ArrayBuffer views #40706
Conversation
please add a test! the likely file is |
I was just looking into it and discovered that there is actually a test that supposed to catch this node/test/fixtures/wpt/FileAPI/blob/Blob-constructor.any.js Lines 285 to 299 in dd52c05
I'm not sure how is it not failing, or maybe it's skipped somehow |
Ok looks like test that would have caught this are disabled here |
I was unable to enable constructor test because there was dependency on |
Try running |
@aduh95 all issues have been addressed |
@@ -220,3 +220,18 @@ assert.throws(() => new Blob({}), { | |||
}); | |||
}); | |||
} | |||
|
|||
(async () => { | |||
const blob = new Blob([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have a comment referencing where this test is coming from.
Commit Queue failed- Loading data for nodejs/node/pull/40706 ✔ Done loading data for nodejs/node/pull/40706 ----------------------------------- PR info ------------------------------------ Title fix: Blob constructor on various ArrayBuffer views (#40706) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch Gozala:patch-1 -> nodejs:master Labels buffer, author ready, commit-queue-squash Commits 8 - fix: Blob constructor on various ArrayBuffer views - fix: only copy necessary bytes - chore: add test case - fix: slice buffer from both sides - Update test/parallel/test-blob.js - fix: address lint & test errors - fix: lint error at test/parallel/test-blob.js - chore: fix test failures for big endian hosts Committers 1 - GitHub PR-URL: https://github.com/nodejs/node/pull/40706 Reviewed-By: Robert Nagy Reviewed-By: Antoine du Hamel Reviewed-By: Minwoo Jung Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/40706 Reviewed-By: Robert Nagy Reviewed-By: Antoine du Hamel Reviewed-By: Minwoo Jung Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 02 Nov 2021 21:28:04 GMT ✔ Approvals: 4 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/40706#pullrequestreview-798208662 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/40706#pullrequestreview-798326412 ✔ - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/40706#pullrequestreview-798468938 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40706#pullrequestreview-816263801 ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2021-12-10T15:19:04Z: https://ci.nodejs.org/job/node-test-pull-request/41451/ - Querying data for job/node-test-pull-request/41451/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 40706 From https://github.com/nodejs/node * branch refs/pull/40706/merge -> FETCH_HEAD ✔ Fetched commits as ef7a686ed996..df6ad221669f -------------------------------------------------------------------------------- Auto-merging lib/internal/blob.js [master 6f3fb5ad28] fix: Blob constructor on various ArrayBuffer views Author: Irakli Gozalishvili Date: Tue Nov 2 14:24:41 2021 -0700 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging lib/internal/blob.js [master e88738c090] fix: only copy necessary bytes Author: Irakli Gozalishvili Date: Tue Nov 2 14:32:36 2021 -0700 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging test/parallel/test-blob.js [master c43f351acb] chore: add test case Author: Irakli Gozalishvili Date: Tue Nov 2 21:57:24 2021 +0000 1 file changed, 15 insertions(+) Auto-merging lib/internal/blob.js [master df00b952b1] fix: slice buffer from both sides Author: Irakli Gozalishvili Date: Wed Nov 3 05:03:33 2021 +0000 1 file changed, 4 insertions(+), 4 deletions(-) Auto-merging test/parallel/test-blob.js [master 11380e6481] Update test/parallel/test-blob.js Author: Irakli Gozalishvili Date: Wed Nov 3 08:57:24 2021 -0700 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging lib/internal/blob.js Auto-merging test/parallel/test-blob.js [master 92f521986c] fix: address lint & test errors Author: Irakli Gozalishvili Date: Wed Nov 3 08:58:24 2021 -0700 2 files changed, 4 insertions(+), 4 deletions(-) Auto-merging test/parallel/test-blob.js [master 66df80bb8d] fix: lint error at test/parallel/test-blob.js Author: Irakli Gozalishvili Date: Wed Nov 3 14:06:22 2021 -0700 1 file changed, 1 insertion(+), 1 deletion(-) Auto-merging test/parallel/test-blob.js [master e9ee2b5b9a] chore: fix test failures for big endian hosts Author: Irakli Gozalishvili Date: Mon Nov 8 12:55:39 2021 -0800 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 8 commits in the PR. Attempting to fixup everything into first commit. [master f5479c4aa0] fix: Blob constructor on various ArrayBuffer views Author: Irakli Gozalishvili Date: Tue Nov 2 14:24:41 2021 -0700 2 files changed, 19 insertions(+), 4 deletions(-) --------------------------------- New Message ---------------------------------- fix: Blob constructor on various ArrayBuffer viewshttps://github.com/nodejs/node/actions/runs/1565588702 |
Landed in 2e1fa8a |
Fixes: nodejs#40705 PR-URL: nodejs#40706 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Node.js v16.14.0 included [a fix](nodejs/node#40706) that meant that `@web-std/blob` started using the Node.js native version. Bad news, Node.js currently copies the buffer on every iteration when obtaining a stream from `File.stream()`. It also has a fixed and small chunk size of `65536` bytes. This makes reading the stream VERY slow and this test fails because it times out. I opened an issue about this here: nodejs/node#42108 Once the test was fixed, the cloudflare build for the website started failing so I had to update next.js to v12. WTF!?! After that, the client build started failing with: ``` Error: Build failed with 2 errors: ../../node_modules/parse-link-header/index.js:3:17: error: Could not resolve "querystring" (use "platform: 'node'" when building for node) ../../node_modules/parse-link-header/index.js:4:18: error: Could not resolve "url" (use "platform: 'node'" when building for node) at failureErrorWithLog (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1493:15) at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1151:28 at runOnEndCallbacks (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:941:63) at buildResponseToResult (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1149:7) at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1258:14 at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:629:9 at handleIncomingPacket (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:726:9) at Socket.readFromStdout (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:596:7) at Socket.emit (node:events:520:28) at addChunk (node:internal/streams/readable:315:12) { errors: [ { detail: undefined, location: [Object], notes: [], pluginName: '', text: `Could not resolve "querystring" (use "platform: 'node'" when building for node)` }, { detail: undefined, location: [Object], notes: [], pluginName: '', text: `Could not resolve "url" (use "platform: 'node'" when building for node)` } ], warnings: [] } ``` So I had to roll the update to `parse-link-header` from #1032 into here as well.
Fixes #40705