CJS exports inside if/else blocks are not exposed to ESM code about jsdelivr HOT 10 OPEN

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.

from jsdelivr.

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'.

from jsdelivr.

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 🤷‍♂️

from jsdelivr.

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] ❌

from jsdelivr.

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.

from jsdelivr.

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?

from jsdelivr.

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.

from jsdelivr.

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!

from jsdelivr.

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

from jsdelivr.

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.

from jsdelivr.