fix #2721: make comment-preservation more general

CHANGELOG.md

@@ -1,5 +1,22 @@
 # Changelog
 
+## Unreleased
+
+* Preserve some comments in expressions ([#2721](https://github.com/evanw/esbuild/issues/2721))
+
+    Various tools give semantic meaning to comments embedded inside of expressions. For example, Webpack and Vite have special "magic comments" that can be used to affect code splitting behavior:
+
+    ```js
+    import(/* webpackChunkName: "foo" */ '../foo');
+    import(/* @vite-ignore */ dynamicVar);
+    new Worker(/* webpackChunkName: "bar" */ new URL("../bar.ts", import.meta.url));
+    new Worker(new URL('./path', import.meta.url), /* @vite-ignore */ dynamicOptions);
+    ```
+
+    Since esbuild can be used as a preprocessor for these tools (e.g. to strip TypeScript types), it can be problematic if esbuild doesn't do additional work to try to retain these comments. Previously esbuild special-cased Webpack comments in these specific locations in the AST. But Vite would now like to use similar comments, and likely other tools as well.
+
+    So with this release, esbuild now will attempt to preserve some comments inside of expressions in more situations than before. This behavior is mainly intended to preserve these special "magic comments" that are meant for other tools to consume, although esbuild will no longer only preserve Webpack-specific comments so it should now be tool-agnostic. There is no guarantee that all such comments will be preserved (especially when `--minify-syntax` is enabled). So this change does *not* mean that esbuild is now usable as a code formatter. In particular comment preservation is more likely to happen with leading comments than with trailing comments. You should put comments that you want to be preserved *before* the relevant expression instead of after it. Also note that this change does not retain any more statement-level comments than before (i.e. comments not embedded inside of expressions). Comment preservation is not enabled when `--minify-whitespace` is enabled (which is automatically enabled when you use `--minify`).
+
 ## 0.16.13
 
 * Publish a new bundle visualization tool

internal/bundler_tests/bundler_default_test.go

@@ -7548,3 +7548,190 @@ func TestMetafileVeryLongExternalPaths(t *testing.T) {
 		},
 	})
 }
+
+func TestCommentPreservation(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				console.log(
+					import(/* before */ foo),
+					import(/* before */ 'foo'),
+					import(foo /* after */),
+					import('foo' /* after */),
+				)
+
+				console.log(
+					require(/* before */ foo),
+					require(/* before */ 'foo'),
+					require(foo /* after */),
+					require('foo' /* after */),
+				)
+
+				console.log(
+					require.resolve(/* before */ foo),
+					require.resolve(/* before */ 'foo'),
+					require.resolve(foo /* after */),
+					require.resolve('foo' /* after */),
+				)
+
+				let [/* foo */] = [/* bar */];
+				let [
+					// foo
+				] = [
+					// bar
+				];
+				let [/*before*/ ...s] = [/*before*/ ...s]
+				let [... /*before*/ s2] = [... /*before*/ s2]
+
+				let { /* foo */ } = { /* bar */ };
+				let {
+					// foo
+				} = {
+					// bar
+				};
+				let { /*before*/ ...s3 } = { /*before*/ ...s3 }
+				let { ... /*before*/ s4 } = { ... /*before*/ s4 }
+
+				let [/* before */ x] = [/* before */ x];
+				let [/* before */ x2 /* after */] = [/* before */ x2 /* after */];
+				let [
+					// before
+					x3
+					// after
+				] = [
+					// before
+					x3
+					// after
+				];
+
+				let { /* before */ y } = { /* before */ y };
+				let { /* before */ y2 /* after */ } = { /* before */ y2 /* after */ };
+				let {
+					// before
+					y3
+					// after
+				} = {
+					// before
+					y3
+					// after
+				};
+				let { /* before */ [y4]: y4 } = { /* before */ [y4]: y4 };
+				let { [/* before */ y5]: y5 } = { [/* before */ y5]: y5 };
+				let { [y6 /* after */]: y6 } = { [y6 /* after */]: y6 };
+
+				foo[/* before */ x] = foo[/* before */ x]
+				foo[x /* after */] = foo[x /* after */]
+
+				console.log(
+					// before
+					foo,
+					/* comment before */
+					bar,
+					// comment after
+				)
+
+				console.log([
+					// before
+					foo,
+					/* comment before */
+					bar,
+					// comment after
+				])
+
+				console.log({
+					// before
+					foo,
+					/* comment before */
+					bar,
+					// comment after
+				})
+
+				console.log(class {
+					// before
+					foo
+					/* comment before */
+					bar
+					// comment after
+				})
+
+				console.log(
+					() => { return /* foo */ null },
+					() => { throw /* foo */ null },
+					() => { return (/* foo */ null) + 1 },
+					() => { throw (/* foo */ null) + 1 },
+					() => {
+						return (// foo
+							null) + 1
+					},
+					() => {
+						throw (// foo
+							null) + 1
+					},
+				)
+
+				console.log(
+					/*a*/ a ? /*b*/ b : /*c*/ c,
+					a /*a*/ ? b /*b*/ : c /*c*/,
+				)
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+			OutputFormat: config.FormatCommonJS,
+			ExternalSettings: config.ExternalSettings{
+				PreResolve: config.ExternalMatchers{
+					Exact: map[string]bool{"foo": true},
+				},
+			},
+		},
+	})
+}
+
+func TestCommentPreservationTransformJSX(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.jsx": `
+				console.log(
+					<div x={/*before*/x} />,
+					<div x={/*before*/'y'} />,
+					<div x={/*before*/true} />,
+					<div {/*before*/...x} />,
+					<div>{/*before*/x}</div>,
+					<>{/*before*/x}</>,
+				)
+			`,
+		},
+		entryPaths: []string{"/entry.jsx"},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+		},
+	})
+}
+
+func TestCommentPreservationPreserveJSX(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.jsx": `
+				console.log(
+					<div x={/*before*/x} />,
+					<div x={/*before*/'y'} />,
+					<div x={/*before*/true} />,
+					<div {/*before*/...x} />,
+					<div>{/*before*/x}</div>,
+					<>{/*before*/x}</>,
+				)
+			`,
+		},
+		entryPaths: []string{"/entry.jsx"},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+			JSX: config.JSXOptions{
+				Preserve: true,
+			},
+		},
+	})
+}

internal/bundler_tests/snapshots/snapshots_dce.txt

@@ -131,6 +131,7 @@ TestConstValueInliningNoBundle
 ---------- /out/top-level.js ----------
 const n_keep = null, u_keep = void 0, i_keep = 1234567, f_keep = 123.456, s_keep = "";
 console.log(
+  // These are doubled to avoid the "inline const/let into next statement if used once" optimization
   null,
   null,
   void 0,
@@ -147,6 +148,7 @@ console.log(
 {
   const s_keep = "";
   console.log(
+    // These are doubled to avoid the "inline const/let into next statement if used once" optimization
     null,
     null,
     void 0,
@@ -164,6 +166,7 @@ console.log(
 function nested() {
   const s_keep = "";
   console.log(
+    // These are doubled to avoid the "inline const/let into next statement if used once" optimization
     null,
     null,
     void 0,
@@ -1586,8 +1589,14 @@ var single_at_no = /* @__PURE__ */ foo(bar());
 var new_single_at_no = /* @__PURE__ */ new foo(bar());
 var single_num_no = /* @__PURE__ */ foo(bar());
 var new_single_num_no = /* @__PURE__ */ new foo(bar());
-var bad_no = foo(bar);
-var new_bad_no = new foo(bar);
+var bad_no = (
+  /* __PURE__ */
+  foo(bar)
+);
+var new_bad_no = (
+  /* __PURE__ */
+  new foo(bar)
+);
 var parens_no = foo(bar);
 var new_parens_no = new foo(bar);
 var exp_no = /* @__PURE__ */ foo() ** foo();