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: expose exports conditions to loaders #31303

Closed
wants to merge 1 commit into from
Closed

module: expose exports conditions to loaders #31303

wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Jan 10, 2020 •
edited
Loading

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 c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 10, 2020
@jkrems
Copy link
Contributor Author

jkrems commented Jan 10, 2020

/cc @nodejs/modules-active-members

addaleax
addaleax reviewed Jan 11, 2020
View reviewed changes
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor

Nice work, this approach seems great!

@sparebytes sparebytes mentioned this pull request Jan 21, 2020
Closed
1 task
@jkrems jkrems force-pushed the loader-conditions branch 9 times, most recently from d05c96e to 66ebe58 Compare February 10, 2020 18:13
@jkrems jkrems marked this pull request as ready for review February 10, 2020 18:18
@jkrems
Copy link
Contributor Author

jkrems commented Feb 10, 2020

Cleaned up and marking as ready for review. I updated the implementation to use a std::set instead of V8::Set. In the process the conditions cache got moved into C++, JS only creates the key (\0-separated condition names).

guybedford
guybedford reviewed Feb 10, 2020
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.

Seems like a nice approach.

src/module_wrap.cc Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/loader-with-custom-condition.mjs Outdated Show resolved Hide resolved
@jkrems jkrems force-pushed the loader-conditions branch from 66ebe58 to 7c15d59 Compare March 2, 2020 18:02
@jkrems jkrems added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Mar 2, 2020
@jkrems
Copy link
Contributor Author

jkrems commented Mar 2, 2020

Ping @nodejs/modules-active-members, this is still lacking some reviews/approvals before we can land it. Adding it to the agenda in case it's still open by our next meeting. :)

bmeck
bmeck approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

I really like this PR! This falls into some things we are starting to see from the community about custom conditions like https://twitter.com/sebmarkbage/status/1234223477424447488 .

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jkrems jkrems added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
guybedford
guybedford approved these changes Mar 3, 2020
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.

Would be great to see this moving forward :)

src/module_wrap.cc Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/loader-with-custom-condition.mjs Outdated Show resolved Hide resolved
bmeck
bmeck reviewed Mar 3, 2020
View reviewed changes
doc/api/esm.md Show resolved Hide resolved
@jkrems jkrems force-pushed the loader-conditions branch from 8703335 to 19b2f4f Compare March 17, 2020 17:42
@nodejs nodejs deleted a comment from jkrems Mar 20, 2020
guybedford
guybedford approved these changes Mar 23, 2020
View reviewed changes
lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved
lib/internal/modules/esm/resolve.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30219/ (:heavy_check_mark:)

@jkrems jkrems force-pushed the loader-conditions branch from 19b2f4f to a14ba1e Compare March 30, 2020 16:16
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jkrems jkrems force-pushed the loader-conditions branch from a14ba1e to 8108485 Compare March 30, 2020 16:31
@jkrems jkrems force-pushed the loader-conditions branch from 8108485 to 2267d17 Compare March 30, 2020 17:13
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jkrems jkrems force-pushed the loader-conditions branch from 2267d17 to 1549bc4 Compare March 31, 2020 16:34
@nodejs-github-bot
Copy link
Collaborator

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

@jkrems
Copy link
Contributor Author

jkrems commented Mar 31, 2020

There's one C++ test that seems to fail on linuxone. I look into it briefly but I don't see a clear reason why it would fail (or what the failure really means). I'm surprised that this PR would affect low-level worker interactions but can't rule it out.

@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

@jkrems See #32563 – I just landed a fix for that. :) Is this still ready to land, apart from waiting for a green CI?

@nodejs-github-bot
Copy link
Collaborator

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

@jkrems
Copy link
Contributor Author

jkrems commented Apr 2, 2020 •
edited
Loading

Is this still ready to land, apart from waiting for a green CI?

Yes! This should be good to go, assuming CI comes back green.

(And thanks for being on top of these build failures, much appreciated. :))

@nodejs-github-bot
Copy link
Collaborator

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

addaleax pushed a commit that referenced this pull request Apr 2, 2020
@jkrems @addaleax
module: expose exports conditions to loaders
9129ab1 
PR-URL: #31303
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

Landed in 9129ab1

@addaleax addaleax closed this Apr 2, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@jkrems @BethGriggs
module: expose exports conditions to loaders
0800b17 
PR-URL: #31303
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
@jkrems @targos
module: expose exports conditions to loaders
537d2c1 
PR-URL: #31303
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
Merged
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@jkrems @targos
module: expose exports conditions to loaders
561ad2a 
PR-URL: nodejs#31303
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@jkrems @targos
module: expose exports conditions to loaders
b62910c 
PR-URL: #31303
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himself65 himself65 himself65 left review comments

@addaleax addaleax addaleax left review comments

@bmeck bmeck bmeck approved these changes

@ljharb ljharb ljharb left review comments

@guybedford guybedford guybedford approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@jkrems @guybedford @nodejs-github-bot @GeoffreyBooth @bmeck @addaleax @ljharb @himself65 @MylesBorins @targos