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: recommend package exports instead of requiring folders #41381

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 2, 2022

Clarify that importing a folder is not supported by default, and add Legacy status to nudge users into using the more modern and versatile "exports" and/or "imports" fields in their package.json.

//cc @nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Jan 2, 2022
doc/api/modules.md Outdated Show resolved Hide resolved
@bmeck
Copy link
Member

bmeck commented Jan 2, 2022

This likely needs a warning that it will block access to adjacent things?

@ljharb
Copy link
Member

ljharb commented Jan 2, 2022

Also, that this feature is the only practical way to make adding “exports” non-breaking.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 2, 2022

This likely needs a warning that it will block access to adjacent things?

Subpath imports does not encapsulate the package AFAIK; subpath exports does, but its documentation warns about that. Given that, I would say a warning is not needed here, but happy to reconsider if you disagree.

Also, that this feature is the only practical way to make adding “exports” non-breaking.

Hum, not sure about that, a more prudent approach is to always treat adding "exports" as a breaking change, I don't think we want the docs to give the impression it's not.

Co-authored-by: Tobias Nießen <tniessen@users.noreply.github.com>
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41381
✔  Done loading data for nodejs/node/pull/41381
----------------------------------- PR info ------------------------------------
Title      doc: recommend package exports instead of requiring folders (#41381)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:no-require-folder -> nodejs:master
Labels     doc, module, author ready
Commits    2
 - doc: recommend package exports instead of requiring folders
 - Update doc/api/modules.md
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41381
Reviewed-By: James M Snell 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41381
Reviewed-By: James M Snell 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 02 Jan 2022 18:05:47 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41381#pullrequestreview-854803616
   ✔  - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/41381#pullrequestreview-854846838
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
   50b7ab9659..f8f3d35bd6  master     -> origin/master
✔  origin/master is now up-to-date
master is out of sync with origin/master. Mismatched commits:
 - 5c0b717ab6 tools: add missing `.PHONY` and `.NOTPARALLEL` targets in `Makefile`
 - f8f3d35bd6 tools: add missing `.PHONY` and `.NOTPARALLEL` targets in `Makefile`
--------------------------------------------------------------------------------
HEAD is now at f8f3d35bd6 tools: add missing `.PHONY` and `.NOTPARALLEL` targets in `Makefile`
   ✔  Reset to origin/master
- Downloading patch for 41381
From https://github.com/nodejs/node
 * branch                  refs/pull/41381/merge -> FETCH_HEAD
✔  Fetched commits as f8f3d35bd639..9969b1c6ab44
--------------------------------------------------------------------------------
Auto-merging doc/api/modules.md
[master dff4d2532d] doc: recommend package exports instead of requiring folders
 Author: Antoine du Hamel 
 Date: Sun Jan 2 19:02:38 2022 +0100
 1 file changed, 10 insertions(+), 4 deletions(-)
Auto-merging doc/api/modules.md
[master 09b7fb7b5e] Update doc/api/modules.md
 Author: Antoine du Hamel 
 Date: Sun Jan 2 21:26:27 2022 +0100
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: recommend package exports instead of requiring folders

PR-URL: #41381
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

[detached HEAD 72225b675e] doc: recommend package exports instead of requiring folders
Author: Antoine du Hamel duhamelantoine1995@gmail.com
Date: Sun Jan 2 19:02:38 2022 +0100
1 file changed, 10 insertions(+), 4 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update doc/api/modules.md

Co-authored-by: Tobias Nießen tniessen@users.noreply.github.com

PR-URL: #41381
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Geoffrey Booth webadmin@geoffreybooth.com

[detached HEAD 1d82ee977f] Update doc/api/modules.md
Author: Antoine du Hamel duhamelantoine1995@gmail.com
Date: Sun Jan 2 21:26:27 2022 +0100
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/1714154234

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 18, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2022
@nodejs-github-bot nodejs-github-bot merged commit 56679eb into nodejs:master Jan 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 56679eb

@aduh95 aduh95 deleted the no-require-folder branch January 18, 2022 18:14
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
PR-URL: #41381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
PR-URL: #41381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41381
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants