add back warning for #466

evanw · Apr 22, 2023 · 81cb21c · 81cb21c
1 parent 0776a4b
commit 81cb21c
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 0 deletions.
diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go
@@ -32,6 +32,7 @@ const (
 	MsgID_JS_PrivateNameWillThrow
 	MsgID_JS_SemicolonAfterReturn
 	MsgID_JS_SuspiciousBooleanNot
+	MsgID_JS_SuspiciousDefine
 	MsgID_JS_ThisIsUndefinedInESM
 	MsgID_JS_UnsupportedDynamicImport
 	MsgID_JS_UnsupportedJSXComment
@@ -129,6 +130,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel)
 		overrides[MsgID_JS_SemicolonAfterReturn] = logLevel
 	case "suspicious-boolean-not":
 		overrides[MsgID_JS_SuspiciousBooleanNot] = logLevel
+	case "suspicious-define":
+		overrides[MsgID_JS_SuspiciousDefine] = logLevel
 	case "this-is-undefined-in-esm":
 		overrides[MsgID_JS_ThisIsUndefinedInESM] = logLevel
 	case "unsupported-dynamic-import":
@@ -245,6 +248,8 @@ func MsgIDToString(id MsgID) string {
 		return "semicolon-after-return"
 	case MsgID_JS_SuspiciousBooleanNot:
 		return "suspicious-boolean-not"
+	case MsgID_JS_SuspiciousDefine:
+		return "suspicious-define"
 	case MsgID_JS_ThisIsUndefinedInESM:
 		return "this-is-undefined-in-esm"
 	case MsgID_JS_UnsupportedDynamicImport:

diff --git a/pkg/api/api_impl.go b/pkg/api/api_impl.go
@@ -600,6 +600,51 @@ func validateDefines(
 		// Define simple expressions
 		if defineExpr.Constant != nil || len(defineExpr.Parts) > 0 {
 			rawDefines[key] = config.DefineData{DefineExpr: &defineExpr}
+
+			// Try to be helpful for common mistakes
+			if len(defineExpr.Parts) == 1 && key == "process.env.NODE_ENV" {
+				data := logger.MsgData{
+					Text: fmt.Sprintf("%q is defined as an identifier instead of a string (surround %q with quotes to get a string)", key, value),
+				}
+				part := defineExpr.Parts[0]
+
+				switch logger.API {
+				case logger.CLIAPI:
+					data.Location = &logger.MsgLocation{
+						File:       "<cli>",
+						Line:       1,
+						Column:     30,
+						Length:     len(part),
+						LineText:   fmt.Sprintf("--define:process.env.NODE_ENV=%s", part),
+						Suggestion: fmt.Sprintf("\\\"%s\\\"", part),
+					}
+
+				case logger.JSAPI:
+					data.Location = &logger.MsgLocation{
+						File:       "<js>",
+						Line:       1,
+						Column:     34,
+						Length:     len(part) + 2,
+						LineText:   fmt.Sprintf("define: { 'process.env.NODE_ENV': '%s' }", part),
+						Suggestion: fmt.Sprintf("'\"%s\"'", part),
+					}
+
+				case logger.GoAPI:
+					data.Location = &logger.MsgLocation{
+						File:       "<go>",
+						Line:       1,
+						Column:     50,
+						Length:     len(part) + 2,
+						LineText:   fmt.Sprintf("Define: map[string]string{\"process.env.NODE_ENV\": \"%s\"}", part),
+						Suggestion: fmt.Sprintf("\"\\\"%s\\\"\"", part),
+					}
+				}
+
+				log.AddMsgID(logger.MsgID_JS_SuspiciousDefine, logger.Msg{
+					Kind: logger.Warning,
+					Data: data,
+				})
+			}
 			continue
 		}
 

diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js
@@ -5863,6 +5863,20 @@ let transformTests = {
     assert.strictEqual(code, `(() => {\n  const import_meta = {};\n  console.log(import_meta, import_meta.foo);\n})();\n`)
   },
 
+  async defineIdentifierVsStringWarningIssue466({ esbuild }) {
+    const { warnings } = await esbuild.transform(``, { define: { 'process.env.NODE_ENV': 'production' } })
+    const formatted = await esbuild.formatMessages(warnings, { kind: 'warning' })
+    assert.strictEqual(formatted[0],
+      `▲ [WARNING] "process.env.NODE_ENV" is defined as an identifier instead of a string (surround "production" with quotes to get a string) [suspicious-define]
+
+    <js>:1:34:
+      1 │ define: { 'process.env.NODE_ENV': 'production' }
+        │                                   ~~~~~~~~~~~~
+        ╵                                   '"production"'
+
+`)
+  },
+
   async json({ esbuild }) {
     const { code } = await esbuild.transform(`{ "x": "y" }`, { loader: 'json' })
     assert.strictEqual(code, `module.exports = { x: "y" };\n`)