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: move package.import content higher #35535

Closed
wants to merge 1 commit into from
Closed

doc: move package.import content higher #35535

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Oct 7, 2020 •
edited
Loading

This is currently at the end of the doc, it likely should be found
right after the documentation about package.exports. Refactored the
docs while duplicating content as little as possible.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 7, 2020
guybedford
guybedford approved these changes Oct 7, 2020
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md

Unlike the `"exports"` field, import maps permit mapping to external packages,
providing an important use case for conditional loading scenarios.
Import maps permit mapping to external packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is already included above.

Suggested change
Import maps permit mapping to external packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this is duplicated content I was trying to keep the most important stuff available in the reference doc

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Oct 7, 2020
View reviewed changes
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

Trott
Trott reviewed Oct 7, 2020
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Oct 7, 2020
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Oct 7, 2020
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Oct 7, 2020
View reviewed changes
doc/api/packages.md Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Contributor Author

I'm very confused about the linting error we are getting in CI. I can't reproduce locally and the line number I'm getting from the CI linter doesn't have any links in it (the error is link related)

@GeoffreyBooth
Copy link
Member

I’m very confused about the linting error we are getting in CI. I can’t reproduce locally and the line number I’m getting from the CI linter doesn’t have any links in it (the error is link related)

It seems to make sense to me . . . ? It’s erroring on this line:

This field defines [subpath imports][] for the current package.

And it appears we don’t have a subpath imports link, only a subpath exports.

@MylesBorins @guybedford
@aduh95 @Trott
doc: move package.import content higher
f1c4999 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>
MylesBorins added a commit that referenced this pull request Oct 9, 2020
@MylesBorins
doc: move package.import content higher
8d8e06a 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: #35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins
Copy link
Contributor Author

Landed in 8d8e06a

@MylesBorins MylesBorins closed this Oct 9, 2020
MylesBorins added a commit that referenced this pull request Oct 14, 2020
@MylesBorins
doc: move package.import content higher
7d1cdd4 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: #35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 14, 2020
Merged
MylesBorins added a commit that referenced this pull request Nov 3, 2020
@MylesBorins
doc: move package.import content higher
4697ad1 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: #35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins added a commit that referenced this pull request Nov 4, 2020
@MylesBorins
doc: move package.import content higher
7eacac4 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: #35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MylesBorins added a commit that referenced this pull request Nov 16, 2020
@MylesBorins
doc: move package.import content higher
1cd1d01 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: #35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@MylesBorins @joesepi
doc: move package.import content higher
fd63d03 
This is currently at the end of the doc, it likely should be found
right after the documentation about `package.exports`. Refactored the
docs while duplicating content as little as possible.

Co-authored-by: Guy Bedford <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Rich Trott <[email protected]>
Signed-off-by: Myles Borins <[email protected]>

PR-URL: nodejs#35535
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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

@ryzokuken ryzokuken ryzokuken approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@guybedford guybedford guybedford approved these changes

@mcollina mcollina mcollina 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.
9 participants
@MylesBorins @nodejs-github-bot @GeoffreyBooth @mcollina @guybedford @Trott @benjamingr @ryzokuken @aduh95