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: unify package exports test for CJS and ESM #28831

Closed
wants to merge 1 commit into from
Closed

module: unify package exports test for CJS and ESM #28831

wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Jul 24, 2019 •
edited
Loading

Refs: nodejs/modules#358

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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 Jul 24, 2019
@jkrems jkrems added esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. labels Jul 24, 2019
@jkrems jkrems changed the title module: unify test suite for CJS and ESM module: unify package exports test for CJS and ESM Jul 24, 2019
guybedford
guybedford approved these changes Jul 24, 2019
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.

Great to have unified tests indeed.

src/module_wrap.cc Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
test/common/index.mjs Show resolved Hide resolved
test/fixtures/node_modules/pkgexports/package.json Show resolved Hide resolved
@jkrems
Copy link
Contributor Author

jkrems commented Jul 24, 2019

/cc @nodejs/modules-active-members

@jkrems jkrems mentioned this pull request Jul 24, 2019
Closed
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 26, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/24724/ ✔️

@jkrems jkrems added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 26, 2019
@jkrems jkrems mentioned this pull request Jul 29, 2019
Closed
@Trott
Copy link
Member

Trott commented Jul 30, 2019

This could use more reviews. @nodejs/collaborators

@guybedford
Copy link
Contributor

More approvals would be great, but this should still be good to merge given the 7 days now I believe.

Trott
Trott requested changes Jul 31, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

abort/test-addon-uv-handle-leak is failing locally for me with this change. Will re-run CI while I investigate locally.... Could be something incompatible slipped in while this PR was baking....

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jul 31, 2019

Looks like I just needed to do a more thorough rebuild to catch up my addons. I think this is good although out of caution, I'll wait for the CI re-run results.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jul 31, 2019

Landed in 61f3a5c

@Trott Trott closed this Jul 31, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 31, 2019
@jkrems @Trott
module: unify package exports test for CJS and ESM
61f3a5c 
Refs: nodejs/modules#358

PR-URL: nodejs#28831
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Aug 2, 2019
@jkrems @targos
module: unify package exports test for CJS and ESM
d9084d2 
Refs: nodejs/modules#358

PR-URL: #28831
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Aug 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengjiawen gengjiawen gengjiawen left review comments

@guybedford guybedford guybedford approved these changes

@Trott Trott Trott 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++. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jkrems @nodejs-github-bot @Trott @guybedford @gengjiawen