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

module: package exports dot main support #29494

Closed
wants to merge 4 commits into from
Closed

module: package exports dot main support #29494

wants to merge 4 commits into from

Conversation

guybedford
Copy link
Contributor

This reintroduces the dot main in exports as discussed in the previous Node.js modules meeting.

The implementation includes both CommonJS and ES module resolution with the associated documentation and resolver specification changes.

In addition to the dot main, "exports" as a string or direct fallback array is supported as well.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 8, 2019
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

GeoffreyBooth
GeoffreyBooth approved these changes Sep 8, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth mentioned this pull request Sep 8, 2019
Closed
2 tasks
guybedford and others added 3 commits September 8, 2019 15:29
@guybedford
linting fix
a9e9bb5
@guybedford @GeoffreyBooth
Update doc/api/esm.md
690237d 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@guybedford @GeoffreyBooth
Update doc/api/esm.md
9f7bc64 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@benjamingr
Copy link
Member

Random governance question - who is supposed to approve these changes? Modules team members or core collaborators?

Has the TSC chartered the module team to make changes to this area of the code on its own or something similar?

(I am asking because I see two approvals by non-collaborator members and I am a bit confused if those LGTMs are "I agree with this change and it can land" "I agree with this change" or "This is what the team said" and if regular collaborators are expected to review those or not)

@devsnek
Copy link
Member

devsnek commented Sep 8, 2019

cc @MylesBorins ^

@guybedford
Copy link
Contributor Author

@benjamingr the standard Node.js core approval process applies since the modules group is not chartered. We typically seek consensus from the modules group before landing non-patch changes to the module system though.

@benjamingr
Copy link
Member

Thanks for explaining @guybedford!

jkrems
jkrems approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

LGTM in general but I'd prefer if we can remove the duplication between the code for . and other exports keys.

src/module_wrap.cc
target =
exports_obj->Get(env->context(), dot_string).ToLocalChecked();
}
if (target->IsString()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to reuse the existing logic for string/array in PackageExportsResolve?

Copy link
Contributor Author

@guybedford guybedford Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That would be better, it's just a refactoring I haven't had the time for. Would you be ok with moving that to a follow-up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me.

@guybedford
Copy link
Contributor Author

It's worth noting here, that:

{
  "exports": "./esm-main.js"
}

is supported by this PR, but:

{
  "exports": "esm-main.js"
}

would not be supported and would throw an error that exports must start with ./.

This is based on various long-standing discussions, but still worth noting in how it deviates from "main".

@jkrems jkrems mentioned this pull request Sep 11, 2019
Closed
MylesBorins
MylesBorins approved these changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 16, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25498/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25502/

@jkrems
Copy link
Contributor

jkrems commented Sep 16, 2019

🍏

@Trott
Copy link
Member

Trott commented Sep 18, 2019

Landed in 3f3ad38

@Trott Trott closed this Sep 18, 2019
Trott pushed a commit that referenced this pull request Sep 18, 2019
@guybedford @GeoffreyBooth @Trott
module: reintroduce package exports dot main
3f3ad38 
This reintroduces the dot main in exports as discussed in the previous
Node.js modules meeting.

The implementation includes both CommonJS and ES module resolution with
the associated documentation and resolver specification changes.

In addition to the dot main, "exports" as a string or direct fallback
array is supported as well.

Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
PR-URL: #29494
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2019
@guybedford @GeoffreyBooth @targos
module: reintroduce package exports dot main
4b3b4ff 
This reintroduces the dot main in exports as discussed in the previous
Node.js modules meeting.

The implementation includes both CommonJS and ES module resolution with
the associated documentation and resolver specification changes.

In addition to the dot main, "exports" as a string or direct fallback
array is supported as well.

Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
PR-URL: #29494
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@guybedford @GeoffreyBooth @BridgeAR
module: reintroduce package exports dot main
ce45aae 
This reintroduces the dot main in exports as discussed in the previous
Node.js modules meeting.

The implementation includes both CommonJS and ES module resolution with
the associated documentation and resolver specification changes.

In addition to the dot main, "exports" as a string or direct fallback
array is supported as well.

Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
PR-URL: #29494
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@aduh95 aduh95 mentioned this pull request Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkrems jkrems jkrems approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@guybedford @benjamingr @devsnek @nodejs-github-bot @jkrems @Trott @ljharb @GeoffreyBooth @MylesBorins