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

Good but stagnant PRs from joyent/node #77

Closed
brendanashworth opened this issue Dec 5, 2014 · 4 comments · May be fixed by olegnn/node#29, erdun/node#34 or saeedahassan/node#28
Closed

Good but stagnant PRs from joyent/node #77

brendanashworth opened this issue Dec 5, 2014 · 4 comments · May be fixed by olegnn/node#29, erdun/node#34 or saeedahassan/node#28

Comments

@brendanashworth
Copy link
Contributor

Through my attempt to contribute to Node.js that resulted in practically no attention, I realized that there are a lot of unknown but good pull requests waiting on the Node.js repository, some of them sizzling for over a year. I understand that some pull requests fly under the radar or get abandoned, but I'd estimate at least half of Node.js's 240 are stagnant.

I would suggest we look for some way to implement some of these good pull requests, but I'm not exactly sure how to approach this.

Here are some searches that reveal pull requests that may never be dealt with:

@chrisdickinson
Copy link
Contributor

Oh, man, I really wanted to comment on this last night, but ran out of steam before I got to it.

This issue is near and dear to my ❤️. I tried to get a read on how to effectively go about this over here: nodebugme/discussion#5.

The biggest help would be to compile a list of these PRs with the following information:

  1. Does the PR actually address what it intends to address?
  2. Is the PR already addressed by subsequent commits?
  3. Did someone known to work on core bless the PR, or is it still up for debate?
  4. How much work (in hours) do you estimate are left on the PR, taking into account pending comments, the PR applying cleanly, etc. Unresolved design issues are the most costly in terms of time, then applying the PR, then style comments, in descending order.

The more eyes, the better. Once we've got a good read on how long a given PR will take to merge, we can make a decision about whether it's better to take it to the finish line or to close it as abandoned (apologetically!) Ideally these would be fulfilled against joyent/node and then iojs can cherry-pick the resulting commits.

The other half of this is to make sure that iojs' tracker never gets to that point, and that discussion is happening over here.

@mikeal
Copy link
Contributor

mikeal commented Dec 5, 2014

Concerns about this have been a source of discussion for some time, since the Node Forward work started actually. I'll try to distill down the main talking points:

  • Effort should be spent first and foremost responding to PR's made against the new repo. We have a chance here not to burn contributors who had given up on joyent/node but that requires that we do a much better job here.
  • It might be better to ask someone to re-submit the PR themselves than do the work of porting it and opening it ourselves. Walking through the process of re-engaging and seeing the difference in response is something that could help them stick with the project and continue to engage.
  • If there are great PR's where the author is not interested in re-submitting themselves then, at some point, we should just pull them in ourselves.

Hope that helps :)

@a0viedo
Copy link
Member

a0viedo commented Dec 8, 2014

There are some good typo-fixing/doc-improving PRs in that stash. Should I start asking them to re-submit the PR to io.js?

@chrisdickinson
Copy link
Contributor

@a0viedo I would start by asking if they'd like someone to adopt their PR, or if they're still interested in maintaining that PR themselves, and cc'ing me on the issue. I can review/shepherd in the commit into node and iojs from there.

kunalspathak referenced this issue in nodejs/node-chakracore Dec 7, 2015
Recently "CollectGarbage()" method on global object was added under a switch.
So instead use JsCollectGarbage API.
Removed usage of JSRT_HAS_NEW_APIs

Fixes: #77.
Reviewed-By: @jianchun
targos added a commit to targos/node that referenced this issue Apr 17, 2021
Original commit message:

    [LTS-M86][builtins] Fix Array.prototype.concat with @@species

    (cherry picked from commit 7989e04979c3195e60a6814e8263063eb91f7b47)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1195977
    Change-Id: I16843bce2e9f776abca0f2b943b898ab5e597e42
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2810787
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#73842}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823829
    Commit-Queue: Jana Grill <janagrill@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Victor-Gabriel Savu <vsavu@google.com>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#77}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8ebd894
targos added a commit to targos/node that referenced this issue Apr 27, 2021
Original commit message:

    [LTS-M86][builtins] Fix Array.prototype.concat with @@species

    (cherry picked from commit 7989e04979c3195e60a6814e8263063eb91f7b47)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1195977
    Change-Id: I16843bce2e9f776abca0f2b943b898ab5e597e42
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2810787
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#73842}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823829
    Commit-Queue: Jana Grill <janagrill@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Victor-Gabriel Savu <vsavu@google.com>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#77}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8ebd894
targos added a commit that referenced this issue Apr 30, 2021
Original commit message:

    [LTS-M86][builtins] Fix Array.prototype.concat with @@species

    (cherry picked from commit 7989e04979c3195e60a6814e8263063eb91f7b47)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1195977
    Change-Id: I16843bce2e9f776abca0f2b943b898ab5e597e42
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2810787
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#73842}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823829
    Commit-Queue: Jana Grill <janagrill@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Victor-Gabriel Savu <vsavu@google.com>
    Cr-Commit-Position: refs/branch-heads/8.6@{#77}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8ebd894

PR-URL: #38275
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment