Skip to content
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

Merged
merged 8 commits into from
Dec 10, 2021
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Nov 2, 2021

Fixes #40705

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 2, 2021
lib/internal/blob.js Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Nov 2, 2021

please add a test! the likely file is tests/parallel/test-blob.js

@Gozala
Copy link
Contributor Author

Gozala commented Nov 2, 2021

please add a test! the likely file is tests/parallel/test-blob.js

I was just looking into it and discovered that there is actually a test that supposed to catch this

test_blob(function() {
return new Blob([
new Uint8Array([0x50, 0x41, 0x53, 0x53]),
new Int8Array([0x50, 0x41, 0x53, 0x53]),
new Uint16Array([0x4150, 0x5353]),
new Int16Array([0x4150, 0x5353]),
new Uint32Array([0x53534150]),
new Int32Array([0x53534150]),
new Float32Array([0xD341500000])
]);
}, {
expected: "PASSPASSPASSPASSPASSPASSPASS",
type: "",
desc: "Passing typed arrays as elements of the blobParts array should work."
});

I'm not sure how is it not failing, or maybe it's skipped somehow

@Gozala
Copy link
Contributor Author

Gozala commented Nov 2, 2021

Ok looks like test that would have caught this are disabled here
https://github.com/nodejs/node/blob/master/test/wpt/status/FileAPI/blob.json

@Gozala
Copy link
Contributor Author

Gozala commented Nov 2, 2021

I was unable to enable constructor test because there was dependency on FileReader API, instead I just ported that test to tests/parallel/test-blob.js.

@aduh95
Copy link
Contributor

aduh95 commented Nov 3, 2021

Try running make lint-js-fix to fix the linter errors. To test if your test is passing before pushing, you can run make -j4 && tools/test.py test/parallel/test-blob.

lib/internal/blob.js Outdated Show resolved Hide resolved
lib/internal/blob.js Outdated Show resolved Hide resolved
test/parallel/test-blob.js Outdated Show resolved Hide resolved
test/parallel/test-blob.js Outdated Show resolved Hide resolved
test/parallel/test-blob.js Outdated Show resolved Hide resolved
test/parallel/test-blob.js Outdated Show resolved Hide resolved
@Gozala
Copy link
Contributor Author

Gozala commented Nov 3, 2021

@aduh95 all issues have been addressed

@ronag ronag requested review from jasnell and aduh95 November 4, 2021 20:02
@ronag ronag added buffer Issues and PRs related to the buffer subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Nov 4, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2021
@nodejs-github-bot
Copy link
Collaborator

@@ -220,3 +220,18 @@ assert.throws(() => new Blob({}), {
});
});
}

(async () => {
const blob = new Blob([
Copy link
Contributor

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.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 10, 2021
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 10, 2021
@nodejs-github-bot
Copy link
Collaborator

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 views

Fixes #40705

PR-URL: #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

[master f9a08f516e] fix: Blob constructor on various ArrayBuffer views
Author: Irakli Gozalishvili contact@gozala.io
Date: Tue Nov 2 14:24:41 2021 -0700
2 files changed, 19 insertions(+), 4 deletions(-)
✖ f9a08f516e6d5288ee3ff071c19c23c0eae2d8ce
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Invalid subsystem: "fix" subsystem
✖ 0:6 First word after subsystem(s) in title should be lowercase. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/1565588702

@aduh95 aduh95 merged commit 2e1fa8a into nodejs:master Dec 10, 2021
@aduh95
Copy link
Contributor

aduh95 commented Dec 10, 2021

Landed in 2e1fa8a

danielleadams pushed a commit that referenced this pull request Dec 13, 2021
Fixes: #40705
    
PR-URL: #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>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
Fixes: #40705
    
PR-URL: #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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Fixes: #40705
    
PR-URL: #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>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Fixes: #40705
    
PR-URL: #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>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Fixes: #40705
    
PR-URL: #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>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
alanshaw pushed a commit to web3-storage/web3.storage that referenced this pull request Feb 24, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new Blob([new Uint16Array([1])]) has size 1 instead of 2
9 participants