esbuild export patterns about cjs-module-lexer

[guybedford]

@evanw the reason the first case is not supported is because we only perform this check at the very top level, in order not to match more than we need, eg in bundling patterns etc. A slight variation does work though, see #46.

I'm still not 100% sold on the hint pattern due to potential minification issues, but you can definitely rely on it working in all previous versions of Node.js and future versions of this project at least.

[evanw]

I don't think I would recommend doing this. The identifiers of local helper functions aren't necessarily stable. For example, __export may be renamed to something like __export2 avoid a name collision if there is already another local variable called __export. They can also be shortened if the code is minified. Neither of these affect patterns like Object.defineProperty(exports, 'q', { enumerable: true, get() { return q } }) which is already supported from what I understand. I could also imagine changing esbuild's code generation for CommonJS exports at some point which would then break this.

My current plan for this is to change esbuild's output somehow to support this library when esbuild's platform is set to node. We discussed this in #34. I could either do Object.defineProperty(exports, 'q', { enumerable: true, get() { return q } }) for each export (very verbose) or follow your suggestion of doing something like 0 && (exports.a = exports.b = exports.c = 0); to annotate esbuild's output files for use with this library. I believe this is tracked by evanw/esbuild#442.

[guybedford]

@evanw point taken, although we do support __exportStar for TypeScript output patterns and this has practically helped in real world use cases on npm, which is what this project is designed for. If bundlers follow the same semantics this can survive minification during processing. But certainly agreed it is brittle too.

The point is more that as code is published to npm using these patterns, it would be beneficial for Node.js to support it, and this project is the best shot at that. npm code is typically direct build tool output so would still work - or does esbuild change these outputs in minification mode to something we can't detect?

If you have suggestions as to alternative patterns that we could support please do share. For example, would better support for Object.defineProperties be useful to you in future?

[evanw]

It just seems like this is coupling things too tightly IMO. I think having esbuild annotate the CommonJS exports for node's ESM loader seems less brittle.

After thinking about it more, something like this would be even more compact: 0 && (module.exports = {a, b, c});. It would also be easy to add re-exports using something like 0 && (module.exports = require('fs'), module.exports = {a, b, c});. If dead-code stripping is a concern, it could be changed to something like Math.random() < 1 || (module.exports = require('fs'), module.exports = {a, b, c}); instead perhaps.

By the way, I think the grammar for doing this in the README is missing a (:

-EXPORTS_LITERAL_PROP: (IDENTIFIER  `:` IDENTIFIER)?) | (IDENTIFIER_STRING `:` IDENTIFIER)
+EXPORTS_LITERAL_PROP: (IDENTIFIER (`:` IDENTIFIER)?) | (IDENTIFIER_STRING `:` IDENTIFIER)

or does esbuild change these outputs in minification mode to something we can't detect?

In this case the __export function is a local variable so it would be minified.

[evanw]

Actually it looks like using module.exports = require overwrites the previous re-export for multiple re-exports, so you'd have to use __exportStar(require()) instead. That's not ideal because it could potentially collide with a local variable with the same name, but I can rename esbuild's function to something else to avoid that most of the time. And it looks like module.exports = {} resets some internal state inside this library so I guess re-exports have to come after. So the way to annotate exports for node would then look like this:

0 && (module.exports = {a, b, c}, __exportStar(require('fs')));

However, that strangely still doesn't seem to work. The re-export is missing. This does work though:

0 && (module.exports = {a, b, c});
0 && __exportStar(require('fs'));

Is this a bug in the parser?

[guybedford]

Ok, for now I have skipped adding any of these. The new 1.1.0 update will take about a week to land on Node.js and a further month or so to fully be backported, if there are changes needed before then there's still a chance we can get a patch in.