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

doc: add primordials guidelines #38635

Closed
wants to merge 17 commits into from
Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 11, 2021

In a recent TSC meeting, it was agreed upon a guide on how to use primordials in core could be helpful. I've tried to come up with a list of frequently asked questions, but if you think it lacks some info or if the info presented is missing nuance or anything, please chime in.

@github-actions github-actions bot added the doc Issues and PRs related to the documentations. label May 11, 2021
Copy link
Member

@marsonya marsonya left a comment

Choose a reason for hiding this comment

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

The guide seems to touch all important aspects of primordials in core.
Looks good to me.

There are just a few nits I wanna bring to your attention.
Kindly ignore the nit(s) that are not important.

doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved
doc/guides/usage-of-primordials-in-core.md Outdated Show resolved Hide resolved

### Prototype methods

ECMAScript provides a bunch of methods available on builtin objects that are
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this sentence is necessary. I would expect people reading this would already know what prototype methods are.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 23, 2022

@ljharb that's correct, at the moment no decision was made on the other areas of the codebase, so the status quo still applies there.
That being said, I don't think we need to wait for a decision to be made on other areas, now that the TSC vote has validated that primordials will be used at least on the error path, I think we should land this for the reasons detailled in #38635 (comment).

@GeoffreyBooth
Copy link
Member

A TSC vote has decided that primordials will be used in (a subset of) the codebase

Which subset?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 24, 2022

A TSC vote has decided that primordials will be used in (a subset of) the codebase

Which subset?

That's not yet decided – the TSC has decided to split its decision in separate votes, only one was done so we don't have the full picture yet (sorry if that's taking quite some time). But we already know that we are not getting rid of primordials.

A bit off-topic for this thread I think, but FYI the TL;DR of the TSC vote result (nodejs/TSC#1158 (comment)) is:
If someone volunteers to add primordials to error paths, we are likely to accept it if and only if there are no significant perf regressions; if someone opens a PR to remove primordials from error paths, it would be rejected, unless said primordials was causing a measurable perf regression. Performance is measured using benchmarks in the benchmark/ directory. Code in the error path is defined as any code that runs before (or inside) an inevitable throw statement.

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 24, 2022

Ping @BridgeAR @mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 8, 2022

Due to the current discussions about primordials in general, I would like to not land this until there is a clear way forward.

@BridgeAR can you please confirm if the way forward is clear enough for this to land? If not, could you please clarify what do you expect to change for this to be ready to land?
As Goeffrey said above, not having guidelines to refer to is a real pain when doing work on Node.js core, I'd like to land this ASAP.

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 20, 2022

According to our collaborator guide, the objection doesn't stand if the objector is unresponsive 7 days after a ping:

#### Objections
Collaborators can object to a pull request by using the "Request
Changes" GitHub feature. Dissent comments alone don't constitute an
objection. Any pull request objection must include a clear reason for that
objection, and the objector must remain responsive for further discussion
towards consensus about the direction of the pull request. Where possible,
provide a set of actionable steps alongside the objection.
If the objection is not clear to others, another collaborator can ask an
objecting collaborator to explain their objection or to provide actionable
steps to resolve the objection. If the objector is unresponsive for seven
days after a collaborator asks for clarification, a collaborator may
dismiss the objection.

Based on that, I'm going to dismiss the remaining objection, and mark the PR as author ready PRs that have at least one approval, no pending requests for changes, and a CI started. .

@BridgeAR if you still want to block this PR from landing, please comment, otherwise I plan to land this PR later this week.

@aduh95 aduh95 dismissed BridgeAR’s stale review April 20, 2022 12:19

Collaborator has been unresponsive for more than 7 days.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 26, 2022

Landed in 92f8c03

@aduh95 aduh95 closed this Apr 26, 2022
@aduh95 aduh95 deleted the primordials-guide branch April 26, 2022 23:45
aduh95 added a commit that referenced this pull request Apr 26, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
nodejs-github-bot pushed a commit that referenced this pull request Apr 27, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Apr 28, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
@targos targos mentioned this pull request May 2, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42877
Refs: #38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#38635
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42877
Refs: nodejs/node#38635
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet