FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: unify package exports test for CJS and ESM by hybrist · Pull Request #28831 · nodejs/node · GitHub

/ node Public

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

Closed
hybrist wants to merge 1 commit into
nodejs:masterfrom
hybrist:one-exports-test
ConversationCommits (1)ChecksFiles changed
Closed

module: unify package exports test for CJS and ESM#28831
hybrist wants to merge 1 commit into
nodejs:masterfrom
hybrist:one-exports-test

Conversation

hybrist commented Jul 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 24, 2019
hybrist 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
hybrist commented Jul 24, 2019
View reviewed changes
Comment thread src/module_wrap.cc Outdated
hybrist commented Jul 24, 2019
View reviewed changes
Comment thread test/common/index.mjs Outdated
hybrist force-pushed the one-exports-test branch from 5db4226 to 5d8e3fb Compare July 24, 2019 04:31
hybrist changed the title module: unify test suite for CJS and ESM module: unify package exports test for CJS and ESM Jul 24, 2019
hybrist force-pushed the one-exports-test branch from 5d8e3fb to 84ccea8 Compare July 24, 2019 04:31
guybedford approved these changes Jul 24, 2019
View reviewed changes

guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Great to have unified tests indeed.

Comment thread src/module_wrap.cc Outdated
Comment thread src/module_wrap.cc Outdated
Comment thread test/common/index.mjs Outdated
Comment thread test/fixtures/node_modules/pkgexports/package.json Outdated
hybrist force-pushed the one-exports-test branch from 84ccea8 to aab065b Compare July 24, 2019 14:15

hybrist commented Jul 24, 2019

Copy link
Copy Markdown
Contributor Author

/cc @nodejs/modules-active-members

hybrist mentioned this pull request Jul 24, 2019
Closed

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

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

Copy link
Copy Markdown
Collaborator

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

hybrist added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 26, 2019
hybrist mentioned this pull request Jul 29, 2019
Closed

Trott commented Jul 30, 2019

Copy link
Copy Markdown
Member

This could use more reviews. @nodejs/collaborators

guybedford commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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

Trott requested changes Jul 31, 2019
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jul 31, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Jul 31, 2019

Copy link
Copy Markdown
Member

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 commented Jul 31, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Jul 31, 2019

Copy link
Copy Markdown
Member

Landed in 61f3a5c

Trott closed this Jul 31, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 31, 2019
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
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 mentioned this pull request Aug 6, 2019
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

guybedford guybedford approved these changes
gengjiawen gengjiawen left review comments
Trott Trott approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL