fix a bug with "HandlesImportErrors"

evanw · Jul 15, 2023 · b8db71b · b8db71b
1 parent 9b83e55
commit b8db71b
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 39 deletions.
diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go
@@ -357,7 +357,13 @@ func parseFile(args parseArgs) {
 			result.resolveResults = make([]*resolver.ResolveResult, len(records))
 
 			if len(records) > 0 {
-				resolverCache := make(map[ast.ImportKind]map[string]*resolver.ResolveResult)
+				type cacheEntry struct {
+					resolveResult *resolver.ResolveResult
+					debug         resolver.DebugMeta
+					didLogError   bool
+				}
+
+				resolverCache := make(map[ast.ImportKind]map[string]cacheEntry)
 				tracker := logger.MakeLineColumnTracker(&source)
 
 				for importRecordIndex := range records {
@@ -376,54 +382,70 @@ func parseFile(args parseArgs) {
 					// Cache the path in case it's imported multiple times in this file
 					cache, ok := resolverCache[record.Kind]
 					if !ok {
-						cache = make(map[string]*resolver.ResolveResult)
+						cache = make(map[string]cacheEntry)
 						resolverCache[record.Kind] = cache
 					}
-					if resolveResult, ok := cache[record.Path.Text]; ok {
-						result.resolveResults[importRecordIndex] = resolveResult
-						continue
-					}
 
-					// Run the resolver and log an error if the path couldn't be resolved
-					resolveResult, didLogError, debug := RunOnResolvePlugins(
-						args.options.Plugins,
-						args.res,
-						args.log,
-						args.fs,
-						&args.caches.FSCache,
-						&source,
-						record.Range,
-						source.KeyPath,
-						record.Path.Text,
-						record.Kind,
-						absResolveDir,
-						pluginData,
-					)
-					cache[record.Path.Text] = resolveResult
-
-					// All "require.resolve()" imports should be external because we don't
-					// want to waste effort traversing into them
-					if record.Kind == ast.ImportRequireResolve {
-						if resolveResult != nil && resolveResult.IsExternal {
-							// Allow path substitution as long as the result is external
-							result.resolveResults[importRecordIndex] = resolveResult
-						} else if !record.Flags.Has(ast.HandlesImportErrors) {
-							args.log.AddID(logger.MsgID_Bundler_RequireResolveNotExternal, logger.Warning, &tracker, record.Range,
-								fmt.Sprintf("%q should be marked as external for use with \"require.resolve\"", record.Path.Text))
+					entry, ok := cache[record.Path.Text]
+					if ok {
+						result.resolveResults[importRecordIndex] = entry.resolveResult
+					} else {
+						// Run the resolver and log an error if the path couldn't be resolved
+						resolveResult, didLogError, debug := RunOnResolvePlugins(
+							args.options.Plugins,
+							args.res,
+							args.log,
+							args.fs,
+							&args.caches.FSCache,
+							&source,
+							record.Range,
+							source.KeyPath,
+							record.Path.Text,
+							record.Kind,
+							absResolveDir,
+							pluginData,
+						)
+						entry = cacheEntry{
+							resolveResult: resolveResult,
+							debug:         debug,
+							didLogError:   didLogError,
+						}
+						cache[record.Path.Text] = entry
+
+						// All "require.resolve()" imports should be external because we don't
+						// want to waste effort traversing into them
+						if record.Kind == ast.ImportRequireResolve {
+							if resolveResult != nil && resolveResult.IsExternal {
+								// Allow path substitution as long as the result is external
+								result.resolveResults[importRecordIndex] = resolveResult
+							} else if !record.Flags.Has(ast.HandlesImportErrors) {
+								args.log.AddID(logger.MsgID_Bundler_RequireResolveNotExternal, logger.Warning, &tracker, record.Range,
+									fmt.Sprintf("%q should be marked as external for use with \"require.resolve\"", record.Path.Text))
+							}
+							continue
 						}
-						continue
 					}
 
-					if resolveResult == nil {
+					// Check whether we should log an error every time the result is nil,
+					// even if it's from the cache. Do this because the error may not
+					// have been logged for nil entries if the previous instances had
+					// the "HandlesImportErrors" flag.
+					if entry.resolveResult == nil {
 						// Failed imports inside a try/catch are silently turned into
 						// external imports instead of causing errors. This matches a common
 						// code pattern for conditionally importing a module with a graceful
 						// fallback.
-						if !didLogError && !record.Flags.Has(ast.HandlesImportErrors) {
+						if !entry.didLogError && !record.Flags.Has(ast.HandlesImportErrors) {
+							// Report an error
 							text, suggestion, notes := ResolveFailureErrorTextSuggestionNotes(args.res, record.Path.Text, record.Kind,
-								pluginName, args.fs, absResolveDir, args.options.Platform, source.PrettyPath, debug.ModifiedImportPath)
-							debug.LogErrorMsg(args.log, &source, record.Range, text, suggestion, notes)
-						} else if !didLogError && record.Flags.Has(ast.HandlesImportErrors) {
+								pluginName, args.fs, absResolveDir, args.options.Platform, source.PrettyPath, entry.debug.ModifiedImportPath)
+							entry.debug.LogErrorMsg(args.log, &source, record.Range, text, suggestion, notes)
+
+							// Only report this error once per unique import path in the file
+							entry.didLogError = true
+							cache[record.Path.Text] = entry
+						} else if !entry.didLogError && record.Flags.Has(ast.HandlesImportErrors) {
+							// Report a debug message about why there was no error
 							args.log.AddIDWithNotes(logger.MsgID_Bundler_IgnoredDynamicImport, logger.Debug, &tracker, record.Range,
 								fmt.Sprintf("Importing %q was allowed even though it could not be resolved because dynamic import failures appear to be handled here:",
 									record.Path.Text), []logger.MsgData{tracker.MsgData(js_lexer.RangeOfIdentifier(source, record.ErrorHandlerLoc),
@@ -432,7 +454,7 @@ func parseFile(args parseArgs) {
 						continue
 					}
 
-					result.resolveResults[importRecordIndex] = resolveResult
+					result.resolveResults[importRecordIndex] = entry.resolveResult
 				}
 			}
 		}

diff --git a/internal/bundler_tests/bundler_default_test.go b/internal/bundler_tests/bundler_default_test.go
@@ -8396,3 +8396,34 @@ func TestLineLimitMinified(t *testing.T) {
 		},
 	})
 }
+
+func TestBadImportErrorMessageWithHandlesImportErrorsFlag(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				import('foo')
+				import('foo')
+				import('foo').catch()
+				import('foo').catch()
+
+				import('bar').catch()
+				import('bar').catch()
+				import('bar') // We should get an error report here even though the earlier imports have the "HandlesImportErrors" flag
+				import('bar')
+
+				import('baz').catch()
+				import('baz').catch()
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+		expectedScanLog: `entry.js: ERROR: Could not resolve "foo"
+NOTE: You can mark the path "foo" as external to exclude it from the bundle, which will remove this error. You can also add ".catch()" here to handle this failure at run-time instead of bundle-time.
+entry.js: ERROR: Could not resolve "bar"
+NOTE: You can mark the path "bar" as external to exclude it from the bundle, which will remove this error. You can also add ".catch()" here to handle this failure at run-time instead of bundle-time.
+`,
+	})
+}