-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
I do not think this solves it ref: #48668 (comment) |
Reopened to fix the first commit message error and add test case of URL.createObjectURL(blob) in #48668 |
@debadree25 , In my macos env(M1 MacBook), $ 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? |
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😅 |
Sorry, I don't know how to fix the error check of |
thats fine we can fix it with rebasing do the following
this will open up vim and you can see something like this
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
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 😄 |
5a661b9
to
a3af3cb
Compare
I done |
great! i think one small typo it will be 'lib' instead of 'ib' |
a3af3cb
to
cdf9d1a
Compare
ok |
cdf9d1a
to
121f759
Compare
Removed |
121f759
to
8c60c33
Compare
So sorry for the delays left some comments overall i think this would indeed resolve the i was probably doing it wrong locally |
Also cc @nodejs/whatwg-stream |
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.
lgtm
86df27f
to
dba9615
Compare
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()https://github.com/nodejs/node/actions/runs/5842162142 |
Landing it manually |
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>
Landed in 3224527 |
Thank you for your contribution @bellbind 🥳 |
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>
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>
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>
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>
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>
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>
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>
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>
Add lacked calling resolve() for finish ReadableStream source.pull().
Fixes: #48668
Fixes: #48916
Fixes: #48232
Refs: 8cc1438