GeezerVison

are you 100% sure this change didn’t fix the issue?

Yes, I just double checked.

CITGM (bluebird only on our fastest platform (s390x)): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2231/

@richardlau Do you mind please providing specific steps to reproduce?

Do you mind please providing specific steps to reproduce?

😅

I have git clones of nodejs/node and nodejs/citgm (with npm install already run inside the citgm clone).

• Make the edits in the clone of nodejs/node (vi lib/internal/modules/cjs/loader.js)
• build (make -j128 2>&1 | tee ../build.linux-x64.log)
• install (tools/install.py install '' /home/riclau/sandbox/bin/testnode/) (this is so we can run with CITGM)
• set path (export PATH=/home/riclau/sandbox/bin/testnode/bin:$PATH)
• Change to the CITGM clone directory
• run node bin/citgm.js bluebird

It may be possible to reduce what bluebird is doing into a simpler testcase... From just looking at the code it appears that:

• package.json contains "main": "./js/release/bluebird.js", and lists under devDependencies "bluebird": "^2.9.2",
• tools/build.js's first line is var Promise = require("bluebird");

So I'm guessing that it used to "work" by falling back to the version of bluebird installed under node_modules (as one of the devDependencies) when Node.js couldn't load the non-existant main?

Thanks @richardlau for the replication instructions there - I managed to finally track this down and have updated the PR with the fix.

Basically, for some reason, all package.json files are being detected as containing an undefined "exports" field in these cases. So shifting the check to be based on "exports" not being undefined instead of being a property fixes the issue.

Tracking down why "exports" is being set as an undefined property on the internally loaded package.json objects is likely a good idea - and entirely unrelated to this PR. I'm not sure if it's due to userland code or internal code.

Either way we are good to land with this fix if we can get this fast tracked now.

Assuming @richardlau can verify that this PR fixes the issue, I propose we fast-track.

CITGM (bluebird only): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2232/

Can confirm locally that this appears to fix the issue (and the results so far in https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2232/ are in agreement).

For posterity I also came to a similar discovery about exports existing but being undefined (@guybedford PRed the second commit fix before I could post):

With the following hacky debug (on top of d3f5674):

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 01d96a4..7c256e0 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -435,9 +435,9 @@ function resolveBasePath(basePath, exts, isMain, trailingSlash, request) {

 function trySelf(parentPath, isMain, request) {
   const { data: pkg, path: basePath } = readPackageScope(parentPath) || {};
-  if (!pkg || 'exports' in pkg === false) return false;
+  if (!pkg || !ObjectPrototypeHasOwnProperty(pkg, 'exports')) return false;
   if (typeof pkg.name !== 'string') return false;
-
+  if (pkg.name === 'bluebird') console.warn(pkg);
   let expansion;
   if (request === pkg.name) {
     expansion = '';

I got this out of the CITGM output:

warn:                         | {
warn:                         | name: 'bluebird',
warn:                         | main: './js/release/bluebird.js',
warn:                         | exports: undefined,
warn:                         | type: undefined
warn:                         | }

What is supposed to happen if someone does Object.prototype.exports = {}?

@guybedford the reason is that readPackageScope calls readPackage and that returns an object with four properties, no matter if they are defined in the package.json or not. One of those properties is exports.

@ljharb PTAL - do you still think this needs both checks? i'm gonna land this in a bit otherwise :)

@codebytere if the only reason for the bug was #31427 (comment) - and if readPackage only reads own properties - then both checks are not needed. However, if readPackage reads inherited properties, then i think both checks are still needed.

Would be good to see a test added for this.

Would be good to see a test added for this.

I took the liberty of adding a test. Hope that's OK. Feel free to remote it or modify it or whatever.

@Trott do you not feel this should be backported to #31382 then? My other option was to take out the offending unflag commit but i was planning to pick this back if possible

@Trott do you not feel this should be backported to #31382 then? My other option was to take out the offending unflag commit but i was planning to pick this back if possible

I support putting this in the release rather than taking out the unflagging. But I'm fine with pulling out the unflagging and waiting until the next semver minor if you or anyone else is concerned. If it causes widespread breakage, the burden will fall to the releasers to get out a quick fix, so I'll defer to you on what the best thing to do here is. But I'm +1 on getting this into the release.

Unflagging conditional exports is very important for ESM adoption. Many package maintainers are waiting for conditional exports before they add ESM support to their packages, especially conditional exports in LTS; so I would err on the side of keeping the unflagging if it all possible.

@Trott We can (should?) probably cut out a lot of stuff from the two package.json files in the test fixture (e.g. @petkaantonov 's email address). Maybe in a follow up if it would otherwise hold up landing this.

@Trott We can (should?) probably cut out a lot of stuff from the two package.json files in the test fixture (e.g. @petkaantonov 's email address). Maybe in a follow up if it would otherwise hold up landing this.

Agreed, that should happen (and probably rename it from something other than "bluebird") and also someone should follow up on @ljharb's observation and see if the other check is necessary or not.

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

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

https://ci.nodejs.org/job/node-test-pull-request/28525/ is green. Landing.

Landed in 343ddff...dcba128

Thanks all for the help getting this one sorted.