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.

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

module: unflag conditional exports #31001

Closed

Conversation

@guybedford
Copy link
Contributor

guybedford commented Dec 17, 2019

This PR unflags conditional exports, which is a prerequisite to unflagging experimental modules and backporting to --experimental-modules on 12.x.

Opening this now so that we have time to discuss it thoroughly before January.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 17, 2019

@Trott Trott added the pending label Dec 21, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 21, 2019

Created a pending label to hep indicate that this shouldn't land until the Modules team has taken it up. At least, that's my understanding. Remove the label if I'm wrong.

@Trott
Trott approved these changes Dec 21, 2019
@nodejs-github-bot

This comment has been minimized.

@guybedford guybedford force-pushed the guybedford:unflag-conditional-exports branch from 2eafac8 to d9619d4 Dec 29, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Dec 29, 2019

@Trott thanks for adding a 'pending' label - will use that pattern for PRs in need of group consensus / sign off / final confirmation.

@guybedford guybedford force-pushed the guybedford:unflag-conditional-exports branch 2 times, most recently from a9989d6 to 63a3044 Jan 7, 2020
Copy link
Member

MylesBorins left a comment

LGTM

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment was marked as outdated.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 12, 2020

This needs a rebase.

guybedford added 2 commits Dec 17, 2019
@MylesBorins MylesBorins force-pushed the guybedford:unflag-conditional-exports branch from 6dbf18d to 8f91464 Jan 15, 2020
@nodejs-github-bot

This comment has been minimized.

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Jan 16, 2020

Landed in fc4e413

MylesBorins added a commit that referenced this pull request Jan 16, 2020
PR-URL: #31001
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Jan 20, 2020

What's the recommended fix here? My first impulse is to add a build step to bluebird's "test" script in package.json.

I'm not sure that's going to help for bluebird based on the failure in CITGM (e.g. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2229/nodes=debian9-64/testReport/junit/(root)/citgm/bluebird_v3_7_2/) as the npm install step runs the prepublish script (which calls bluebird's tools/build.js script) and it's that that is throwing the error:

       > bluebird@3.7.2 prepublish /home/iojs/tmp/citgm_tmp/8cac5b63-5c2f-4118-94da-7b6540fd2912/bluebird
 > npm run generate-browser-core && npm run generate-browser-full
 > bluebird@3.7.2 generate-browser-core /home/iojs/tmp/citgm_tmp/8cac5b63-5c2f-4118-94da-7b6540fd2912/bluebird
 > node tools/build.js --features=core --no-debug --release --zalgo --browser --minify && mv js/browser/bluebird.js js/browser/bluebird.core.js && mv js/browser/bluebird.min.js js/browser/bluebird.core.min.js
 internal/modules/cjs/loader.js:323
       throw err;
       ^
 Error: Cannot find module '/home/iojs/tmp/citgm_tmp/8cac5b63-5c2f-4118-94da-7b6540fd2912/bluebird/js/release/bluebird.js'. Please verify that the package.json has a valid "main" entry
     at tryPackage (internal/modules/cjs/loader.js:315:19)
     at resolveBasePath (internal/modules/cjs/loader.js:427:16)
     at trySelf (internal/modules/cjs/loader.js:456:12)
     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:956:22)
     at Function.Module._load (internal/modules/cjs/loader.js:862:27)
     at Module.require (internal/modules/cjs/loader.js:1040:19)
     at require (internal/modules/cjs/helpers.js:72:18)
     at Object.<anonymous> (/home/iojs/tmp/citgm_tmp/8cac5b63-5c2f-4118-94da-7b6540fd2912/bluebird/tools/build.js:1:15)
     at Module._compile (internal/modules/cjs/loader.js:1151:30)
     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1171:10) {
   code: 'MODULE_NOT_FOUND',
   path: '/home/iojs/tmp/citgm_tmp/8cac5b63-5c2f-4118-94da-7b6540fd2912/bluebird/package.json',
   requestPath: 'bluebird'
 }
 npm ERR! code ELIFECYCLE
 npm ERR! errno 1
 npm ERR! bluebird@3.7.2 generate-browser-core: `node tools/build.js --features=core --no-debug --release --zalgo --browser --minify && mv js/browser/bluebird.js js/browser/bluebird.core.js && mv js/browser/bluebird.min.js js/browser/bluebird.core.min.js`
 npm ERR! Exit status 1
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 20, 2020

Yes, tools/test.js contains var Promise = require("bluebird"); and package.json contains "name": "bluebird"

Thanks this helps a lot. Does that same package.json file contain an "exports" key or not? If you notice the trySelf flag line that was removed in this PR, it has an equivalent return false to before this PR if the package.json does not have an "exports" key.

If the package.json does have an "exports" key then this is the opt-in behaviour as designed.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 20, 2020

Will not be around for the next few hours, so if you need urgent debugging perhaps @jkrems or @GeoffreyBooth can advise too.

I don't think it's so urgent that it can't wait a few hours. I'm not sure when @codebytere was hoping to get the release out, but I'm guessing it wouldn't be until at least approximately 24 hours from now.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Jan 20, 2020

Yes, tools/test.js contains var Promise = require("bluebird"); and package.json contains "name": "bluebird"

Thanks this helps a lot. Does that same package.json file contain an "exports" key or not? If you notice the trySelf flag line that was removed in this PR, it has an equivalent return false to before this PR if the package.json does not have an "exports" key.

If the package.json does have an "exports" key then this is the opt-in behaviour as designed.

It does not contain an exports key.
https://github.com/petkaantonov/bluebird/blob/master/package.json

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 20, 2020

Ok, the only other thing I can think to try is switching the in check at https://github.com/nodejs/node/pull/31001/files#diff-76195ce57689942222a27f0dbda6d3b7R438 to an Object.hasOwnProperty check in case of Object prototype pollution from other code.

@guybedford guybedford mentioned this pull request Jan 20, 2020
2 of 4 tasks complete
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 20, 2020

I've created #31427 as a possible fix.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Jan 20, 2020

I don't think this PR has caused the regression but it has surfaced it (by unflagging --experimental-conditional-exports). Running CITGM on bluebird with 13.6.0 (which doesn't contain this PR) and NODE_OPTIONS=--experimental-conditional-exports produces the same failure: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/727/

Trott added a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Jan 21, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Jan 21, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* doc:
  * add GeoffreyBooth to collaborators (Geoffrey Booth) #31306
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
codebytere added a commit that referenced this pull request Jan 21, 2020
Notable changes:

* deps:
  * upgrade to libuv 1.34.1 (cjihrig) #31332
  * upgrade npm to 6.13.6 (Ruy Adorno) #31304
* module
  * add API for interacting with source maps (bcoe) #31132
  * loader getSource, getFormat, transform hooks (Geoffrey Booth) #30986
  * logical conditional exports ordering (Guy Bedford) #31008
  * unflag conditional exports (Guy Bedford) #31001
* process:
  * allow monitoring uncaughtException (Gerhard Stoebich) #31257
* Added new collaborators:
  * [GeoffreyBooth](https://github.com/GeoffreyBooth) - Geoffrey Booth. #31306

PR-URL: #31382
MylesBorins added a commit that referenced this pull request Jan 28, 2020
PR-URL: #31001
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 28, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 28, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit to MylesBorins/node that referenced this pull request Jan 30, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit to MylesBorins/node that referenced this pull request Jan 30, 2020
Refs: nodejs#31001 (comment)

PR-URL: nodejs#31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 30, 2020
PR-URL: #31001
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 30, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 30, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this pull request Feb 6, 2020
PR-URL: #31001
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
BethGriggs added a commit that referenced this pull request Feb 6, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this pull request Feb 6, 2020
Refs: #31001 (comment)

PR-URL: #31427
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@targos targos mentioned this pull request Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.