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

Link to WinterCG Runtime Keys in export conditions docs #48408

Merged
merged 1 commit into from Jun 20, 2023
Merged

Link to WinterCG Runtime Keys in export conditions docs #48408

merged 1 commit into from Jun 20, 2023

Conversation

jcbhmr
Copy link
Contributor

@jcbhmr jcbhmr commented Jun 10, 2023 •
edited

This PR would...

  • Add a link to https://runtime-keys.proposal.wintercg.org/ in the export conditions section
  • Remove "deno" condition from Node.js docs
  • Remove "react-native" condition?
  • Keep "types" condition?
  • Add a "user would expect it to be in Node.js core docs" criteria to add more conditions to the list

cc @guybedford
follow up of #48407

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 10, 2023
aduh95
aduh95 reviewed Jun 10, 2023
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
@jcbhmr jcbhmr marked this pull request as ready for review June 10, 2023 07:17
aduh95
aduh95 reviewed Jun 10, 2023
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
JakobJingleheimer
JakobJingleheimer reviewed Jun 10, 2023
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
guybedford
guybedford approved these changes Jun 10, 2023
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for working this through

doc/api/packages.md Outdated Show resolved Hide resolved
@jcbhmr jcbhmr changed the title Link to runtime keys in export conditions docs Link to WinterCG Runtime Keys in export conditions docs Jun 10, 2023
anonrig
anonrig reviewed Jun 10, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I feel like we should wait for the proposal to be finalized before merging this.

@guybedford
Copy link
Contributor

@Ethan-Arrowood would be great to have your review on this one.

anonrig
anonrig reviewed Jun 10, 2023
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
Ethan-Arrowood
Ethan-Arrowood approved these changes Jun 10, 2023
View reviewed changes
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

LGTM - fwiw WinterCG is a W3C Community Group. We can't actually "publish" specifications/standards yet beyond "draft". I think runtime keys is in a stable enough place to be referenced here.

@anonrig
Copy link
Member

anonrig commented Jun 10, 2023

LGTM - fwiw WinterCG is a W3C Community Group. We can't actually "publish" specifications/standards yet beyond "draft". I think runtime keys is in a stable enough place to be referenced here.

Thanks @Ethan-Arrowood. We are using the word proposal in the reference and in the URL (https://runtime-keys.proposal.wintercg.org), and the website contains draft, as well as your message. Which one is the correct terminology to define the spec? Shouldn't we be aligned on both node.js docs, runtime-keys URL, and the runtime-keys website content (referring Draft Community Group Report, 14 April 2023)

@Ethan-Arrowood
Copy link
Contributor

Proposal and draft are synonymous I believe. I'm not a stickler about it

@anonrig
Copy link
Member

anonrig commented Jun 11, 2023

@jcbhmr can you fix the linter issues?

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Jun 11, 2023

It was that one of the lines was a bit too long. 😆 I think I've fixed it! 👍

@anonrig
Copy link
Member

anonrig commented Jun 11, 2023

@jcbhmr Can you squash all commits and make sure that the first commit explains what this pull request does? If we add commit-queue-squash label, it will be merged into main branch using the commit message doc: remove "deno" export condition because it is the first commit in this pull request.

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Jun 12, 2023 •
edited

Done! 😊 Now there's a single commit:

[link-to-runtime-keys a3a2cc26cd5] doc: remove non-Node.js export conditions & link to WinterCG Runtime Keys
 Author: Jacob Hummer <jcbhmr@outlook.com>
 1 file changed, 9 insertions(+), 4 deletions(-)

I did this:

git reset --soft origin/main
git commit -m ...

as described here: https://stackoverflow.com/questions/25356810/git-how-to-squash-all-commits-on-branch

@anonrig
Copy link
Member

anonrig commented Jun 12, 2023

Thanks. The commit message is too long. Please take a look at commit message guidelines.

@jcbhmr
doc: link to Runtime Keys in export conditions
ee10a5d 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.
@jcbhmr
Copy link
Contributor Author

jcbhmr commented Jun 12, 2023 •
edited

I've rewritten the commit message. ❤️

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit 43c3d9f into nodejs:main Jun 20, 2023
17 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 43c3d9f

@jcbhmr jcbhmr deleted the link-to-runtime-keys branch June 24, 2023 23:59
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@jcbhmr @RafaelGSS
doc: link to Runtime Keys in export conditions
2a403cd 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.

PR-URL: #48408
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@jcbhmr @Ceres6
doc: link to Runtime Keys in export conditions
d41ba14 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.

PR-URL: nodejs#48408
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@jcbhmr @Ceres6
doc: link to Runtime Keys in export conditions
9bf8ec2 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.

PR-URL: nodejs#48408
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 10, 2023
@jcbhmr @ruyadorno
doc: link to Runtime Keys in export conditions
da5d76e 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.

PR-URL: #48408
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 10, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@jcbhmr @ruyadorno
doc: link to Runtime Keys in export conditions
f8ba672 
This commit:
1. Adds a link to the WinterCG Runtime Keys proposal draft spec in the
    documentation for the `export` condition.
2. Adds a criteria to add more export conditions to the Node.js docs:
    they should be Node.js core-relevant.
3. Removes the "deno" and "react-native" export conditions from the core
    docs with the expectation that readers will follow the link to the
    Runtime Keys proposal draft spec to learn about them and more.

PR-URL: #48408
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@jasnell jasnell jasnell approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@guybedford guybedford guybedford approved these changes

@lpinca lpinca lpinca approved these changes

@anonrig anonrig anonrig approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@tniessen tniessen tniessen approved these changes

@aymen94 aymen94 aymen94 approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet