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

doc: update guidance for adding new modules #44576

Closed
wants to merge 10 commits into from
Closed

doc: update guidance for adding new modules #44576

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b4b1ced
doc: update guidance for adding new modules
mhdawson Sep 8, 2022
9b3ae9a
squash:fixup
mhdawson Sep 8, 2022
e82833b
Update doc/contributing/collaborator-guide.md
Trott Sep 8, 2022
347453e
Update doc/contributing/collaborator-guide.md
Trott Sep 8, 2022
621a8b0
Update doc/contributing/collaborator-guide.md
Trott Sep 8, 2022
d129114
Update doc/contributing/collaborator-guide.md
Trott Sep 8, 2022
af6919e
Update doc/contributing/collaborator-guide.md
Trott Sep 8, 2022
a9fe2fe
Update doc/contributing/collaborator-guide.md
mhdawson Sep 9, 2022
0b19d4e
Update doc/contributing/collaborator-guide.md
mhdawson Sep 9, 2022
c524819
Update doc/contributing/collaborator-guide.md
mhdawson Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 14 additions & 6 deletions doc/contributing/collaborator-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,13 +392,21 @@ metadata. Raise a pull request like any other change.

Treat commits that introduce new core modules with extra care.

Check if the module's name conflicts with an existing ecosystem module. If it
does, choose a different name unless the module owner has agreed in writing to
transfer it.
New modules must be added with the `node:` prefix.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is a little ambiguous. Are you saying based on the TSC vote that new modules should be available only with the node: prefix? If so, I think we should clarify that point (since core modules are already available with the node: prefix by default).

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig thanks, incorporated this and your other comments.

mhdawson marked this conversation as resolved.
Show resolved Hide resolved

If the new module name is free, register a placeholder in the module registry as
soon as possible. Link to the pull request that introduces the new core module
in the placeholder's `README`.
When adding promises to an existing API, add `/promises`
(`inspector/promises`, etc.). Apply the `semver-major` label to the addition.
Copy link
Member

@ljharb ljharb Sep 13, 2022 •
edited

Choose a reason for hiding this comment

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

if node added a semver-major change that prevented any slashed requires/imports from core module names, then this category would no longer be semver-major after that.

(edit: oops, this was mentioned here: #44576 (comment) )

Copy link
Member

Choose a reason for hiding this comment

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

I created #44651 for that.


If the new module name without the `node:` prefix is free, register
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to call out node: here since it is never available on npm. What about this?

If the new module name is available in the module registry, reserve it as soon as possible.

mhdawson marked this conversation as resolved.
Show resolved Hide resolved
a placeholder in the module registry as soon as possible. Link to the pull
request that introduces the new core module in the placeholder's `README`.

If the module name without the `node:` prefix is not free and the module is
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about not needing to mention node:.

mhdawson marked this conversation as resolved.
Show resolved Hide resolved
not widely used, contact the owner to see if they would be willing to transfer
it to the project.

targos marked this conversation as resolved.
Show resolved Hide resolved
We try to register a placeholder without the `node:` prefix whenever
Trott marked this conversation as resolved.
Show resolved Hide resolved
possible to avoid confusion and typosquatting attacks.

For pull requests introducing new core modules:

Expand Down