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

lib: fix to add resolve() before return at Blob.stream()'s source.pull() #48935

Closed
wants to merge 2 commits into from

Conversation

bellbind
Copy link
Contributor

@bellbind bellbind commented Jul 27, 2023

Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438

@debadree25
Copy link
Member

I do not think this solves it ref: #48668 (comment)

@bellbind
Copy link
Contributor Author

Reopened to fix the first commit message error and add test case of URL.createObjectURL(blob) in #48668

@bellbind
Copy link
Contributor Author

bellbind commented Jul 27, 2023

@debadree25 , In my macos env(M1 MacBook), make test prints All tests passed.

$ make test
....
[05:18|% 100|+ 3901|-   0]: Done

All tests passed.
$

and especially:

$ ./out/Release/node --expose-internals test/parallel/test-blob.js --expose-internals
$

Which test file failed do you think?

@debadree25
Copy link
Member

Weird in my device i had seen atleast 2 failed tests lets wait for github actions run to complete, also would please apply the updates to this PR only, one PR is easier to maintain instead of multiple ones😅

@bellbind
Copy link
Contributor Author

Sorry, I don't know how to fix the error check of the first commit message to adding commits into same branch.
So I add a new pull request with a updated first commit message.

@debadree25
Copy link
Member

thats fine we can fix it with rebasing do the following

git rebase -i HEAD~1

this will open up vim and you can see something like this

pick 5a661b9828 lib/internal/blob.js: fix Blob.stream() for #48668 #48916

# Rebase 1eae568a76..5a661b9828 onto 1eae568a76 (1 command)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
#                    keep only this commit's message; -c is same as -C but
#                    opens the editor
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
#         create a merge commit using the original merge commit's
#         message (or the oneline, if no original merge commit was
#         specified); use -c <commit> to reword the commit message
# u, update-ref <ref> = track a placeholder for the <ref> to be updated
#                       to this position in the new commits. The <ref> is
#                       updated at the end of the rebase
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.

now go to insert mode in vim by typing i go to the first line and type r instead of pick then escape and :wq from vim you will next be greeted with this

ib: fix Blob.stream() for #48668 #48916

Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Author:    bellbind <bellbind@gmail.com>
# Date:      Wed Jul 26 22:24:02 2023 +0900
#
# interactive rebase in progress; onto 1eae568a76
# Last command done (1 command done):
#    reword 5a661b9828 lib/internal/blob.js: fix Blob.stream() for #48668 #48916
# No commands remaining.
# You are currently editing a commit while rebasing branch 'pullreq-fix-lib-internal-blob' on '1eae568a76'.
#
# Changes to be committed:
#       modified:   lib/internal/blob.js
#       modified:   test/parallel/test-blob.js
#
# Untracked files:
#       test.mjs
#       test/parallel/test-readablestream-from.js
#       test1.mjs
#       test2.mjs
#       test3.mjs
#       test4.mjs
#
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
~                                                                                                                                                                     
-- INSERT --

here rewrite the commit message and proceed to exit vim and then do git push -f this should rebase and fix the commit

although we can do this later too, or i can do it for you too we can first get he issue fixed and all and then fix this 😄

@bellbind bellbind force-pushed the pullreq-fix-lib-internal-blob branch from 5a661b9 to a3af3cb Compare July 27, 2023 07:46
@bellbind
Copy link
Contributor Author

I done git push -f origin HEAD

@debadree25
Copy link
Member

great! i think one small typo it will be 'lib' instead of 'ib'

@bellbind bellbind force-pushed the pullreq-fix-lib-internal-blob branch from a3af3cb to cdf9d1a Compare July 27, 2023 07:49
@bellbind
Copy link
Contributor Author

ok

@bellbind bellbind force-pushed the pullreq-fix-lib-internal-blob branch from cdf9d1a to 121f759 Compare July 27, 2023 07:53
@bellbind
Copy link
Contributor Author

bellbind commented Jul 27, 2023

Removed #48... in commit message that is one of failed reasons of First commit message

@bellbind bellbind force-pushed the pullreq-fix-lib-internal-blob branch from 121f759 to 8c60c33 Compare July 27, 2023 08:00
@bellbind bellbind changed the title lib/internal/blob.js: fix Blob.stream() for #48668 #48916 lib: fix to add resolve() before return at Blob.stream()'s source.pull() Jul 27, 2023
@debadree25
Copy link
Member

So sorry for the delays left some comments overall i think this would indeed resolve the i was probably doing it wrong locally

@debadree25
Copy link
Member

Also cc @nodejs/whatwg-stream

@debadree25 debadree25 added web streams request-ci Add this label to start a Jenkins CI on a PR. labels Jul 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@debadree25 debadree25 force-pushed the pullreq-fix-lib-internal-blob branch from 86df27f to dba9615 Compare August 12, 2023 10:55
@debadree25 debadree25 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023
@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 Aug 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/48935
✔  Done loading data for nodejs/node/pull/48935
----------------------------------- PR info ------------------------------------
Title      lib: fix to add resolve() before return at Blob.stream()'s source.pull() (#48935)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     bellbind:pullreq-fix-lib-internal-blob -> nodejs:main
Labels     author ready, needs-ci, web streams, commit-queue-squash
Commits    2
 - lib: fix to add resolve() before return at Blob.stream()'s source.pull()
 - fixup! test only if crypto is present and an additional test from bug…
Committers 1
 - Debadree Chatterjee 
PR-URL: https://github.com/nodejs/node/pull/48935
Fixes: https://github.com/nodejs/node/issues/48668
Fixes: https://github.com/nodejs/node/issues/48916
Fixes: https://github.com/nodejs/node/pull/48232
Refs: https://github.com/nodejs/node/commit/8cc14387a2e77d7a2b411e7edaed690d43ea3809
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matthew Aitken 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48935
Fixes: https://github.com/nodejs/node/issues/48668
Fixes: https://github.com/nodejs/node/issues/48916
Fixes: https://github.com/nodejs/node/pull/48232
Refs: https://github.com/nodejs/node/commit/8cc14387a2e77d7a2b411e7edaed690d43ea3809
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matthew Aitken 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 27 Jul 2023 02:21:50 GMT
   ✔  Approvals: 4
   ✔  - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/48935#pullrequestreview-1553662906
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/48935#pullrequestreview-1553664563
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/48935#pullrequestreview-1575213696
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/48935#pullrequestreview-1566518697
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-12T10:58:16Z: https://ci.nodejs.org/job/node-test-pull-request/53232/
- Querying data for job/node-test-pull-request/53232/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 48935
From https://github.com/nodejs/node
 * branch                  refs/pull/48935/merge -> FETCH_HEAD
✔  Fetched commits as 87af913b66ea..dba96155a1f1
--------------------------------------------------------------------------------
[main 5e571055a9] lib: fix to add resolve() before return at Blob.stream()'s source.pull()
 Author: bellbind 
 Date: Wed Jul 26 22:24:02 2023 +0900
 2 files changed, 28 insertions(+)
[main 55f5598f08] fixup! test only if crypto is present and an additional test from bug report
 Author: Debadree Chatterjee 
 Date: Fri Aug 11 23:34:30 2023 +0530
 1 file changed, 42 insertions(+), 7 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting to fixup everything into first commit.
[main 5c6095c50a] lib: fix to add resolve() before return at Blob.stream()'s source.pull()
 Author: bellbind 
 Date: Wed Jul 26 22:24:02 2023 +0900
 2 files changed, 63 insertions(+)
   ⚠  Found Fixes: https://github.com/nodejs/node/issues/48668, skipping..
   ⚠  Found Fixes: https://github.com/nodejs/node/issues/48916, skipping..
   ⚠  Found Refs: https://github.com/nodejs/node/commit/8cc14387a2e77d7a2b411e7edaed690d43ea3809, skipping..
--------------------------------- New Message ----------------------------------
lib: fix to add resolve() before return at Blob.stream()'s source.pull()

Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Fixes: #48232
Reviewed-By: Debadree Chatterjee debadree333@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Matthew Aitken maitken033380023@gmail.com

[main 82bf7f4ae4] lib: fix to add resolve() before return at Blob.stream()'s source.pull()
Author: bellbind bellbind@gmail.com
Date: Wed Jul 26 22:24:02 2023 +0900
2 files changed, 63 insertions(+)
✖ 82bf7f4ae4712694a5f918720c6460cc6ec5a0d0
✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer
✔ 3:7 Valid fixes URL. fixes-url
✔ 4:7 Valid fixes URL. fixes-url
✔ 5:7 Valid fixes URL. fixes-url
✖ 8:7 Pull request URL must reference a comment or discussion. 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
✔ 7:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.
Please manually ammend the commit message, by running
git commit --amend
Once commit message is fixed, finish the landing command running
git node land --continue

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

@debadree25
Copy link
Member

Landing it manually

debadree25 pushed a commit that referenced this pull request Aug 12, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@debadree25
Copy link
Member

Landed in 3224527

@debadree25 debadree25 closed this Aug 12, 2023
@debadree25
Copy link
Member

Thank you for your contribution @bellbind 🥳

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
PR-URL: nodejs#48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
PR-URL: nodejs#48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
PR-URL: nodejs#48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: nodejs#48668
Fixes: nodejs#48916
Fixes: nodejs#48232
Refs: nodejs@8cc1438
PR-URL: nodejs#48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
Add lacked calling resolve() for finish ReadableStream source.pull().

Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438
PR-URL: #48935
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
@KhafraDev KhafraDev mentioned this pull request Sep 1, 2023
20 tasks
vasco-santos pushed a commit to w3s-project/w3up that referenced this pull request Sep 6, 2023
This PR reverts the workaround for the
[bug](nodejs/node#48916) in nodejs 20.5 (and
below) which was [resolved](nodejs/node#48935)
in nodejs 20.6.

---------

Co-authored-by: Travis Vachon <travis.vachon@protocol.ai>
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. 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. needs-ci PRs that need a full CI run. web streams
Projects
None yet
7 participants