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

meta: considerations for new core modules #15022

Closed
wants to merge 6 commits into from
Closed

meta: considerations for new core modules #15022

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a17c71
meta: considerations for new core modules
jasnell Aug 24, 2017
101f84c
[Squash] Refine details based on feedback
jasnell Aug 25, 2017
41242d8
[Squash] Nit
jasnell Aug 25, 2017
479aeae
[Squash] Be forceful and assertive. Believe in yourself.
jasnell Aug 25, 2017
fbcd579
[Squash] I think we're getting closer.
jasnell Aug 25, 2017
00df632
[Squash] *Now* I think I've got it.
jasnell Aug 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions COLLABORATOR_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,20 @@ multiple commits. Commit metadata and the reason for the revert should be
appended. Commit message rules about line length and subsystem can be ignored.
A Pull Request should be raised and approved like any other change.

### Introducing New Modules

Semver-minor commits that introduce new core modules should be treated with
extra care.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Would that be a should or a must?

Copy link
Member

Choose a reason for hiding this comment

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

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.


* First, the name of the new core module should not conflict with any existing
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove First, .

Copy link
Member

Choose a reason for hiding this comment

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

There's also singular/plural disagreement in this sentence. Here's what I'd write:

  • The name of the new core module should not conflict with an existing module in the ecosystem unless there is an agreement with the owner of the existing module to transfer ownership.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there should be an email, GitHub comment, or some other paper trail that shows the agreement with the existing module owner?

Copy link
Member Author

Choose a reason for hiding this comment

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

something along those lines yes.. I'm not sure how prescriptive we need to be with it.

module in the ecosystem unless an agreement with the owner of those modules
is reached to transfer ownership.
* PRs that introduce new core modules must be landed as semver-minor commits.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unnecessary to state. Any new feature must be landed as semver-minor. If you're trying to say that even landing as Experimental is semver-minor (which I agree it should be), put that in with the statement about it needing to be Experimental first.

* New core modules must be landed initially as `Experimental` and must go
through at least one release cycle before being moved out of experimental.
Copy link
Member

Choose a reason for hiding this comment

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

The term "release cycle" is probably vague to a lot of people.

Copy link
Member

Choose a reason for hiding this comment

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

So, yeah, putting my comments on the second and third bullet points together, maybe consolidate them into this single bullet point:

  • New core modules must be landed with a Stability Index of Experimental. (Pull requests proposing new core modules must still be marked semver-minor.) New core modules must remain Experimental until a semver-major release.

* It is recommended to give PRs introducing new core modules more time for
review than the typical 48/72 hour review used for other commits.
Copy link
Member

Choose a reason for hiding this comment

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

Suggest a concrete time framer here. I'd go with this:

  • Pull requests introducing new core modules should remain open for one week rather than the usual 48-hour/72-hour review period for other pull requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would suggest longer.... maybe even up to a month?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe phrase it as a minimum? (open for at least one week)

Copy link
Member

Choose a reason for hiding this comment

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

I actually agree with @jasnell as this would probably have already prevented some trouble. Landing a new module should normally not be urgent.
One alternative might also be smth like At least one week without anyone requesting any further changes.?
And I would not make it a recommendation but mandatory with a optional fast path through a decision of the CTC/TSC?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be possible for collaborators to pick an appropriate timeframe, even without a fixed rule. One week as a minimum sounds good, but in any case the gist of it will be “if your PR is large/has profound impact, wait until people have had a fair chance to review & assess it”

Copy link
Member

Choose a reason for hiding this comment

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

Please explain why we don't want to go the EPS route. If a new module isn't a major addition I don't know what is.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax ... "at least one week" sounds good to me but I would point out that the issue that led to me creating this issue in the first place (Jeremiah's concern that I landed the perf_hooks stuff too fast) meets all of the criteria discussed so far in this thread so I'm not sure if it's sufficient to make everyone comfortable. There needs to be a good balance so that a good compromise can be reached.


### Deprecations

Deprecation refers to the identification of Public APIs that should no longer
Expand Down