Skip to content

Commit

Permalink
fix a bug with "HandlesImportErrors"
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Jul 15, 2023
1 parent 9b83e55 commit b8db71b
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 39 deletions.
100 changes: 61 additions & 39 deletions internal/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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),
Expand All @@ -432,7 +454,7 @@ func parseFile(args parseArgs) {
continue
}

result.resolveResults[importRecordIndex] = resolveResult
result.resolveResults[importRecordIndex] = entry.resolveResult
}
}
}
Expand Down
31 changes: 31 additions & 0 deletions internal/bundler_tests/bundler_default_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
`,
})
}

0 comments on commit b8db71b

Please sign in to comment.