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

meta: considerations for new core modules #15022

Closed
wants to merge 6 commits into
base: master
from

Conversation

@jasnell
Member

jasnell commented Aug 24, 2017

@jasnell jasnell added the meta label Aug 24, 2017

Show outdated Hide outdated COLLABORATOR_GUIDE.md
Show outdated Hide outdated COLLABORATOR_GUIDE.md
Show outdated Hide outdated COLLABORATOR_GUIDE.md
Show outdated Hide outdated COLLABORATOR_GUIDE.md
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2017

Member

Updated based on feedback.

  1. @cjihrig ... I clarified that the agreement with the existing module author should be written
  2. @Trott and @addaleax ... I've made the time frame at least one week
  3. @AndreasMadsen ... I've added a recommendation to use EPS under certain conditions

PTAL

Member

jasnell commented Aug 25, 2017

Updated based on feedback.

  1. @cjihrig ... I clarified that the agreement with the existing module author should be written
  2. @Trott and @addaleax ... I've made the time frame at least one week
  3. @AndreasMadsen ... I've added a recommendation to use EPS under certain conditions

PTAL

Show outdated Hide outdated COLLABORATOR_GUIDE.md

@jasnell jasnell referenced this pull request Aug 25, 2017

Closed

perf_hooks: implementation of Performance Timing API #14680

4 of 4 tasks complete
@lpinca

lpinca approved these changes Aug 25, 2017

@Trott

Trott approved these changes Aug 25, 2017

LGTM with or without my last suggestion.

@mcollina

LGTM with my two comments adressed.

The name of the new core module should not conflict with any existing
module in the ecosystem unless a written agreement with the owner of those
modules is reached to transfer ownership.

This comment has been minimized.

@mcollina

mcollina Aug 25, 2017

Member

We need to add a statement like: "if the new module name is free, a collaborator should park it as soon as possible, linking to the pull request that introduces the new module".

@mcollina

mcollina Aug 25, 2017

Member

We need to add a statement like: "if the new module name is free, a collaborator should park it as soon as possible, linking to the pull request that introduces the new module".

This comment has been minimized.

@jasnell

jasnell Aug 25, 2017

Member

oh right, good point

@jasnell

jasnell Aug 25, 2017

Member

oh right, good point

This comment has been minimized.

@mcollina

mcollina Aug 25, 2017

Member

I think you still have not added this.

@mcollina

mcollina Aug 25, 2017

Member

I think you still have not added this.

This comment has been minimized.

@jasnell

jasnell Aug 25, 2017

Member

oy... heh... one sec

@jasnell

jasnell Aug 25, 2017

Member

oy... heh... one sec

### Introducing New Modules
Semver-minor commits that introduce new core modules should be treated with
extra care.

This comment has been minimized.

@mcollina

mcollina Aug 25, 2017

Member

I would add that they need to land with two CTC (or maybe TSC now) signoff, like any other breaking change.

@mcollina

mcollina Aug 25, 2017

Member

I would add that they need to land with two CTC (or maybe TSC now) signoff, like any other breaking change.

This comment has been minimized.

@jasnell

jasnell Aug 25, 2017

Member

Would that be a should or a must?

@jasnell

jasnell Aug 25, 2017

Member

Would that be a should or a must?

This comment has been minimized.

@mcollina

mcollina Aug 25, 2017

Member

I think that's a must. It's serious business: we are adding new API that we will need to maintain to core. Removing those is hard, so the CTC should be notified to weight in.

@mcollina

mcollina Aug 25, 2017

Member

I think that's a must. It's serious business: we are adding new API that we will need to maintain to core. Removing those is hard, so the CTC should be notified to weight in.

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Aug 25, 2017

Member

EPS is an ok for speculative large changes but it's really hasn't been that successful of a process over all. Having a concrete pr available, with real code, real tests, real documentation is far easier to review and look at to determine if this is something we want. For instance, Bradley's ESM pr has been a thousand times for valuable than the endless noisy threads in the EPS repo on it. Same goes for the efforts that led to N-API, URL, Inspector, etc. Yes, some discussion in EPS can be helpful but gating whether or not a semver minor can land based on that seems far too unnecessarily heavyweight of a process and, frankly, discourages innovation significantly. I'd definitely be -1 to requiring EPS and have, on several occasions, even considered proposing removing it.

I guessed that was the reason. Personally, I disagree, when I look at PEP (Python Enhancement Proposal) I'm always impressed by the level of detail and thought that goes into pretty much any major feature. It is super helpful to learn about the thought process that went into the feature. I think we can learn a lot from them.

But if EPS is something we want to discourage, then we should change the EPS guide lines or deprecate the approach completely. But a minimum requirement for any new major feature should be to put it on the CTC agenda, even if there are no disagreements. That would have made any concerns in this cause quite obvious. This is effectively equivalent to the EPS approach, but with less documentation and more code.

Member

AndreasMadsen commented Aug 25, 2017

EPS is an ok for speculative large changes but it's really hasn't been that successful of a process over all. Having a concrete pr available, with real code, real tests, real documentation is far easier to review and look at to determine if this is something we want. For instance, Bradley's ESM pr has been a thousand times for valuable than the endless noisy threads in the EPS repo on it. Same goes for the efforts that led to N-API, URL, Inspector, etc. Yes, some discussion in EPS can be helpful but gating whether or not a semver minor can land based on that seems far too unnecessarily heavyweight of a process and, frankly, discourages innovation significantly. I'd definitely be -1 to requiring EPS and have, on several occasions, even considered proposing removing it.

I guessed that was the reason. Personally, I disagree, when I look at PEP (Python Enhancement Proposal) I'm always impressed by the level of detail and thought that goes into pretty much any major feature. It is super helpful to learn about the thought process that went into the feature. I think we can learn a lot from them.

But if EPS is something we want to discourage, then we should change the EPS guide lines or deprecate the approach completely. But a minimum requirement for any new major feature should be to put it on the CTC agenda, even if there are no disagreements. That would have made any concerns in this cause quite obvious. This is effectively equivalent to the EPS approach, but with less documentation and more code.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2017

Member

@AndreasMadsen ... that's fair :-)

We've been actively moving away from putting things on the ctc-agenda label in favor of a more async decision making process using ctc-review and only putting exceptions on ctc-agenda. It's been working rather well so far. So how about this, we keep the minimum one week, we require two CTC members to sign off (as @mcollina suggests) and we require that the issue is flagged with ctc-review during that time....

(Of course, that's in addition to it landing as Experimental)

Member

jasnell commented Aug 25, 2017

@AndreasMadsen ... that's fair :-)

We've been actively moving away from putting things on the ctc-agenda label in favor of a more async decision making process using ctc-review and only putting exceptions on ctc-agenda. It's been working rather well so far. So how about this, we keep the minimum one week, we require two CTC members to sign off (as @mcollina suggests) and we require that the issue is flagged with ctc-review during that time....

(Of course, that's in addition to it landing as Experimental)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 25, 2017

Member

Ok, take a look now! I think I've captured your feedback @AndreasMadsen and @mcollina :-)

Member

jasnell commented Aug 25, 2017

Ok, take a look now! I think I've captured your feedback @AndreasMadsen and @mcollina :-)

@mcollina

LGTM

@Fishrock123 Fishrock123 self-requested a review Aug 26, 2017

@AndreasMadsen

LGTM

Especially the "ctc-review label" part is important to me.

As for how we should deal with pref_hooks specifically, I will defer that to @Fishrock123. It is clear to me that @Fishrock123 was against the PR and if a collaborator is against a PR then the CTC should be involved, which it wasn't. Mistakes were made, that happens and is fine, but we have to reevaluate perf_hooks as well.

jasnell added a commit that referenced this pull request Aug 29, 2017

meta: considerations for new core modules
PR-URL: #15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2017

Member

Landed in 397dbab

Member

jasnell commented Aug 29, 2017

Landed in 397dbab

@jasnell jasnell closed this Aug 29, 2017

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 29, 2017

Member

Should we notify all collaborators about this change?

Member

targos commented Aug 29, 2017

Should we notify all collaborators about this change?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Aug 29, 2017

ping @nodejs/collaborators ... fyi

module in the ecosystem unless a written agreement with the owner of those
modules is reached to transfer ownership.
If the new module name is free, a Collaborator should register a placeholder

This comment has been minimized.

@jkrems

jkrems Aug 29, 2017

Contributor

Should we commit to adding (or allowing for) polyfills to be published for older versions of node under that name? Or is that out of scope for this generic document and would be discussed case-by-case?

@jkrems

jkrems Aug 29, 2017

Contributor

Should we commit to adding (or allowing for) polyfills to be published for older versions of node under that name? Or is that out of scope for this generic document and would be discussed case-by-case?

This comment has been minimized.

@jasnell

jasnell Aug 29, 2017

Member

I'd say that's out of scope

@jasnell

jasnell Aug 29, 2017

Member

I'd say that's out of scope

oe added a commit to ayojs/ayo that referenced this pull request Aug 30, 2017

meta: considerations for new core modules
PR-URL: nodejs/node#15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

oe added a commit to ayojs/ayo that referenced this pull request Aug 30, 2017

meta: considerations for new core modules
PR-URL: nodejs/node#15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Aug 31, 2017

meta: considerations for new core modules
PR-URL: nodejs#15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

meta: considerations for new core modules
PR-URL: #15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

meta: considerations for new core modules
PR-URL: #15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

meta: considerations for new core modules
PR-URL: #15022
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment