CJS exports inside if/else blocks are not exposed to ESM code

[jcbhmr]

Description

Normally (in Node.js, Bun, or Deno) when you import something that is CJS using an ESM import() or import statement, you get a module object with the .default being the raw module.exports of the CJS module, plus any statically-analyzable exports.thing = value names as names available on the esm object. Here's a quick demo:

// lib.cjs
exports.hello = () => "Hi!"

// app.mjs
import { hello } from "./lib.cjs"
console.log(hello())
//=> "Hi!"

This even works on esm.run!

const m = await import('http://esm.run/@nodefill/util')
console.log(m)
//=> { ... }
// 👆 a big object with lots of keys ON THE MODULE NAMESPACE OBJECT

However, that doesn't appear to be working for this package: https://www.npmjs.com/package/@nodefill/primordials

const m = await import('https://esm.run/@nodefill/primordials')
console.log(m)
//=> { default: { ... } }
// 👆 single default export even though Node.js and Deno recognize the exports!

example of deno vs esm.run:

I don't know how you're currently detecting CJS exports, but I do know that Node.js seems to be using https://github.com/nodejs/cjs-module-lexer (idk what Deno uses)

github repo of npm package https://github.com/nodefill/primordials

Affected jsDelivr links

https://esm.run/@nodefill/primordials

Response headers

https://cdn.jsdelivr.net/npm/@nodefill/primordials/+esm

Information

• Device OS: Windows
• Browser & Version: Chrome 114
• VPN provider (if you use one) NONE
• Your location (country/region) United States

Requisites

• I performed a cursory search of the issue tracker to avoid opening a duplicate issue.
• I checked the documentation to understand that the issue I am reporting is not normal behavior.
• I understand that not filling out this template correctly will lead to the issue being closed.

Additional content

esm.sh also has this problem so you're not alone (this would be a great chance to 1-up them tho 🤣)

[MartinKolarik]

Hi, thanks for the detailed issue.

The problem is most likely that the exports are wrapped in a condition if (typeof primordials !== 'undefined') {, so they are not actually static. We should be able to handle cases where the value of the condition is known in advance, as well as conditions based on process.env.NODE_ENV, but this is not the case here.

If the condition is necessary in some environments, a possible solution might be adding a separate file with really static exports and referencing it under a browser condition in the exports of package.json:

"exports": {
  ".": {
    "browser": "./dist/index-browser.js",
    "deno": "./dist/index-deno+bun+default.js",
    "bun": "./dist/index-deno+bun+default.js",
    "node": "./dist/index-node.js",
    "default": "./dist/index-deno+bun+default.js"
  },
  "./lib/*": null,
  "./*.js": "./dist/*.js"
},

We use the exports in the following order: 'browser', 'module', 'import', 'production', 'default'.

[jcbhmr]

they are not actually static

Both Deno and Node.js disagree with you 🤣
Even if the export will end up as undefined, it's still a named export.
https://github.com/nodejs/cjs-module-lexer

// lib.cjs
if (false) exports.hi = 34

// app.mjs
// DOES NOT throw a syntax error
import { hi } from "./lib.cjs"
console.log(hi)
//=> undefined

I think that there is definite value to aligning with the existing Deno + Node.js static export detection logic even if it's "wrong" idealistically.
btw there's even a grammar for CJS export detection: https://github.com/nodejs/cjs-module-lexer/tree/1.2.2#grammar

---

as for my solution, I think I'm going to end up doing something like this only for browser targets:

exports.named = exports.otherNamed = exports.hi = void 0;

if (typeof primordials !== "undefined") {
  module.exports = primordials;
} else {
  module.exports = {
    hi: require("./hi.js"),
  };
}

but that incurs the additional overhead of duplicating all 760 named exports twice: once redundantly to satisfy under-classifying CJS lexers and once to actually assign them 🤷‍♂️

[jcbhmr]

Actually, that still doesn't work:

Raw text from unpkg: https://unpkg.com/@nodefill/[email protected]/dist/index-browser.js

exports.uncurryThis = exports.applyBind = exports.Proxy =
  exports.globalThis = exports.decodeURI /* ... */ = void 0

if (typeof primordials !== 'undefined') {
  module.exports = primordials;
} else {
  module.exports = {
    ["uncurryThis"]: require("./uncurryThis.js");
    // ...
  }
  Object.setPrototypeOf(module.exports, null);
  Object.freeze(module.exports);
}

esm.run + esm.sh

https://esm.sh has you beaten here! Even they detect it 🤣

https://esm.sh/@nodefill/[email protected] ✅
https://esm.run/@nodefill/[email protected] ❌

[MartinKolarik]

I think that there is definite value to aligning with the existing Deno + Node.js static export detection logic even if it's "wrong" idealistically.

Definitely, I agree that it would be a nice improvement, which we should consider. That will take some time though, so my previous response was aiming for an immediate solution.

Actually, that still doesn't work:

This problem seems like something we can fix quickly. However, looking at the currently supported patterns, I think there's an even better approach that doesn't require duplication and should work right now:

(() => {
  if (typeof primordials !== "undefined") {
    module.exports = primordials;
    return;
  }
  
  module.exports = {
    hi: require("./hi.js"),
    name: require("./name.js"),
  };
})();

The point is having exports outside an if/else block, which is what breaks the detection. IIFEs are supported and combined with the early return it should do the same thing.

[jcbhmr]

You're right, that does work!

unpkg source js:

it even works in esm.run and esm.sh too! Win win!

---

you've troubleshooted my problem and given me an acceptable workaround. i note that you've changed the issue title - do you want me to leave this issue to track if()-CJS stuff, or should it be closed?

[MartinKolarik]

Great! Let's keep it open for anyone who comes across the same issue until we improve the lexer to handle the original case as well.

[jcbhmr]

Sidenote that might warrant another issue: esm.run has the same issue as esm.sh with non-JS identifier exports. These are valid in ES2022:

// Yes, this is valid JavaScript!
import { "RegExpGet$&" as RegExpGet$amp } from "@nodefill/primordials";

console.log(RegExpGet$amp());
//=> '\x00\x00\x00'

☝ yes that works in Node.js and deno (after i prodded them about it denoland/deno#19665 denoland/deno#19679 )

and should be exposed on the module namespace. you at least do better than esm.sh which doesn't expose .eval!

[MartinKolarik]

Ok, that's interesting. I didn't know export { x as "RegExpGet$&" } was actually valid, and my IDE doesn't understand it either, but looking at the spec, it is indeed meant to work. In that case, we can support those exports too.

[jcbhmr]

TypeScript doesn't support it which is probably why it's not very common. microsoft/TypeScript#40594

[MartinKolarik]

Unfortunately, it seems rollup (at least in the version we use) can't handle that syntax either, so the fix isn't as easy as I thought. I'll open a separate issue.