fix #4100: invalid identifiers in node annotation

Author: evanw

CHANGELOG.md

@@ -1,5 +1,39 @@
 # Changelog
 
+## Unreleased
+
+* Fix node-specific annotations for string literal export names ([#4100](https://github.com/evanw/esbuild/issues/4100))
+
+    When node instantiates a CommonJS module, it scans the AST to look for names to expose via ESM named exports. This is a heuristic that looks for certain patterns such as `exports.NAME = ...` or `module.exports = { ... }`. This behavior is used by esbuild to "annotate" CommonJS code that was converted from ESM with the original ESM export names. For example, when converting the file `export let foo, bar` from ESM to CommonJS, esbuild appends this to the end of the file:
+
+    ```js
+    // Annotate the CommonJS export names for ESM import in node:
+    0 && (module.exports = {
+      bar,
+      foo
+    });
+    ```
+
+    However, this feature previously didn't work correctly for export names that are not valid identifiers, which can be constructed using string literal export names. The generated code contained a syntax error. That problem is fixed in this release:
+
+    ```js
+    // Original code
+    let foo
+    export { foo as "foo!" }
+
+    // Old output (with --format=cjs --platform=node)
+    ...
+    0 && (module.exports = {
+      "foo!"
+    });
+
+    // New output (with --format=cjs --platform=node)
+    ...
+    0 && (module.exports = {
+      "foo!": null
+    });
+    ```
+
 ## 0.25.1
 
 * Fix incorrect paths in inline source maps ([#4070](https://github.com/evanw/esbuild/issues/4070), [#4075](https://github.com/evanw/esbuild/issues/4075), [#4105](https://github.com/evanw/esbuild/issues/4105))

internal/bundler_tests/bundler_default_test.go

@@ -1698,6 +1698,27 @@ func TestNodeAnnotationFalsePositiveIssue3544(t *testing.T) {
 	})
 }
 
+// https://github.com/evanw/esbuild/issues/4100
+func TestNodeAnnotationInvalidIdentifierIssue4100(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.mjs": `
+				let foo, bar, baz
+				export {
+					foo, bar as if, baz as "..."
+				}
+			`,
+		},
+		entryPaths: []string{"/entry.mjs"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			OutputFormat:  config.FormatCommonJS,
+			AbsOutputFile: "/out.js",
+			Platform:      config.PlatformNode,
+		},
+	})
+}
+
 func TestMinifiedBundleES6(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler_tests/snapshots/snapshots_default.txt

@@ -5431,6 +5431,27 @@ function confuseNode(exports2) {
   confuseNode
 });
 
+================================================================================
+TestNodeAnnotationInvalidIdentifierIssue4100
+---------- /out.js ----------
+// entry.mjs
+var entry_exports = {};
+__export(entry_exports, {
+  "...": () => baz,
+  foo: () => foo,
+  if: () => bar
+});
+module.exports = __toCommonJS(entry_exports);
+var foo;
+var bar;
+var baz;
+// Annotate the CommonJS export names for ESM import in node:
+0 && (module.exports = {
+  "...": null,
+  foo,
+  if: null
+});
+
 ================================================================================
 TestNodeModules
 ---------- /Users/user/project/out.js ----------

internal/linker/linker.go

@@ -5078,7 +5078,7 @@ func (c *linkerContext) generateEntryPointTailJS(
 
 				// "{if: null}"
 				var valueOrNil js_ast.Expr
-				if _, ok := js_lexer.Keywords[export]; ok {
+				if _, ok := js_lexer.Keywords[export]; ok || !js_ast.IsIdentifier(export) {
 					// Make sure keywords don't cause a syntax error. This has to map to
 					// "null" instead of something shorter like "0" because the library
 					// "cjs-module-lexer" only supports identifiers in this position, and

scripts/end-to-end-tests.js

@@ -2197,14 +2197,18 @@ tests.push(
 
 // Test external ES6 export
 tests.push(
-  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs'], {
+  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--platform=node'], {
     'foo.js': `export const foo = 123`,
     'node.js': `const out = require('./out'); if (!out.__esModule || out.foo !== 123) throw 'fail'`,
   }),
-  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs'], {
+  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--platform=node'], {
     'foo.js': `export default 123`,
     'node.js': `const out = require('./out'); if (!out.__esModule || out.default !== 123) throw 'fail'`,
   }),
+  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--platform=node'], {
+    'foo.js': `const something = 123; export { something as 'some name' }`,
+    'node.js': `const out = require('./out'); if (!out.__esModule || out['some name'] !== 123) throw 'fail'`,
+  }),
   test(['--bundle', 'foo.js', '--outfile=out.js', '--format=esm'], {
     'foo.js': `export const foo = 123`,
     'node.js': `import {foo} from './out.js'; if (foo !== 123) throw 'fail'`,
@@ -2213,6 +2217,10 @@ tests.push(
     'foo.js': `export default 123`,
     'node.js': `import out from './out.js'; if (out !== 123) throw 'fail'`,
   }),
+  test(['--bundle', 'foo.js', '--outfile=out.js', '--format=esm'], {
+    'foo.js': `const something = 123; export { something as 'some name' }`,
+    'node.js': `import { 'some name' as out } from './out.js'; if (out !== 123) throw 'fail'`,
+  }),
   test(['--bundle', 'foo.js', '--outfile=out.js', '--format=cjs', '--platform=node'], {
     'foo.js': `
       export function confuseNode(exports) {