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.

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

Jump to bottom

[v12.x backport] stream: avoid unecessary nextTick #29691

Closed
wants to merge 1 commit into from
Closed

[v12.x backport] stream: avoid unecessary nextTick #29691

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 24, 2019

Refs: #29194

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Sep 24, 2019
@ronag ronag changed the base branch from master to v12.x-staging September 24, 2019 14:58
@ronag
Copy link
Member Author

ronag commented Sep 24, 2019 •
edited
Loading

This is my first backport. I don't understand what Backport-PR-URL: is supposed to contain (step 9.v?). URL to this link or the original PR?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2019

Also I don't believe I can do 9.vi? Can I?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2019

ping @BridgeAR

@ronag ronag force-pushed the backport-29194-to-v12.x branch 2 times, most recently from f8d373c to 24286bf Compare September 24, 2019 15:03
@ronag ronag changed the title [v8.x backport] stream: avoid unecessary nextTick [v12.x backport] stream: avoid unecessary nextTick Sep 24, 2019
@targos
Copy link
Member

targos commented Sep 24, 2019

Backport-PR-URL should contain this PR's url, so:

Backport-PR-URL: https://github.com/nodejs/node/pull/29691

@targos
Copy link
Member

targos commented Sep 24, 2019

It looks like the commit metadata from the original was lost. This should also be in the commit message:

PR-URL: https://github.com/nodejs/node/pull/29194
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@ronag ronag force-pushed the backport-29194-to-v12.x branch from 24286bf to d6b5b56 Compare September 24, 2019 16:37
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25679/

@ronag ronag force-pushed the backport-29194-to-v12.x branch from d6b5b56 to 25b6b2c Compare September 24, 2019 17:40
@ronag
Copy link
Member Author

ronag commented Sep 24, 2019

Unrelated travis failure?

@BridgeAR BridgeAR added the v12.x label Sep 26, 2019
@ronag ronag closed this Oct 4, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2019

@ronag was there a specific reason that you closed the PR? The releasers normally land the backport PRs some time before preparing the next release. Seems like it did not end up in the last release and would need another rebase.

The travis failure should be ignored as long as the Node.js CI passes.

@ronag ronag reopened this Oct 9, 2019
@ronag
stream: avoid unecessary nextTick
bd05183 
If we are not going to emit 'close' then there is no reason to
schedule it.

PR-URL: nodejs#29194
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#29691
@ronag ronag force-pushed the backport-29194-to-v12.x branch from 25b6b2c to bd05183 Compare October 9, 2019 20:14
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26470/

@ronag
Copy link
Member Author

ronag commented Dec 15, 2019 •
edited
Loading

This PR #30851 pretty much makes this redudant. I'll make a new backport port based on #30851 once that lands.

@ronag ronag closed this Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ronag @targos @nodejs-github-bot @BridgeAR