WIP/RFC: Include import stacks in resolution errors #1054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Changelog: * **[Fix]**: Resolution errors now show accurate source locations. D20107057 added code frames to resolution errors. That diff used a heuristic (simple string search for the import specifier) that has some unfortunate failure modes. For instance, if there is another import (say a Flow/TS `import type` declaration) with the same module specifier, the resulting error can falsely suggest that Metro is trying to follow the type import, which is confusing for users. Here, we use the `dependency` object from the resolver context (newly available since D47453640) to instead construct a code frame based on the true location. We fall back to the old heuristic since the location can sometimes be missing. This diff also adds integration tests to prevent regressions. Reviewed By: GijsWeterings Differential Revision: D47471112 fbshipit-source-id: a5e60522621b81b09e2ba2d7510e9510ea882923
Summary: Adds infrastructure for reporting *import stacks* as part of resolution errors in Metro. An import stack is the (inverse) chain of dependencies in the graph that led to a resolution error. This can help users understand and fix build errors that occur inside third-party packages (e.g. because of configuration errors). See expo/expo#23551 for more motivation. ## Caveats Ideally, we'd always be able to print the full chain of dependencies from an entry point to the module containing the error.¹ However, since we update the graph incrementally in response to filesystem changes, and produce import stacks while there are still pending changes to process, we can only safely print edges that we have *just* traversed². Otherwise, it's possible that a dependency's metadata (e.g. source location) will be stale, or indeed that an edge we print doesn't exist anymore. ¹ Also, for UX reasons, we'd want to print the *shortest path* between those two modules. ² Our main traversal is depth-first, so it's also not guaranteed to take the shortest possible path to each module. As a result, **there is an observable difference between** (the import stacks printed by) **initial and incremental builds**. * An error in an initial build (no existing Graph instance) will contain the full import stack, from the module containing the error down to the entry point. * An error in an incremental build (e.g. full reload / Fast Refresh of a bundle that previously built successfully) will contain a *partial* import stack - from the module containing the error down to some ancestor module *M*, where M is in the set of modules added/changed/invalidated since the previous successful build. * In this case we can still reliably tell the user that the dependency chain connects *somehow* to the bundle's entry point (since otherwise the module wouldn't be in the graph). Changelog: * **[Feature]** Include import stacks in resolution errors. --- TODO: * See how this renders in LogBox/RedBox and adjust accordingly. * Decide whether to bake the import stack into the error message or fully rely on external error reporters for presentation. * Probably print project-relative paths rather than absolute paths. * Pretty-print the paths of context modules rather than expose the generated, virtual paths. Differential Revision: D47548085 fbshipit-source-id: 781e3c4c4fc17a4e216e9adea1ab4c0cd708aff0
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D47548085 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Adds infrastructure for reporting import stacks as part of resolution errors in Metro. An import stack is the (inverse) chain of dependencies in the graph that led to a resolution error. This can help users understand and fix build errors that occur inside third-party packages (e.g. because of configuration errors).
See expo/expo#23551 for more motivation.
Caveats
Ideally, we'd always be able to print the full chain of dependencies from an entry point to the module containing the error.¹ However, since we update the graph incrementally in response to filesystem changes, and produce import stacks while there are still pending changes to process, we can only safely print edges that we have just traversed². Otherwise, it's possible that a dependency's metadata (e.g. source location) will be stale, or indeed that an edge we print doesn't exist anymore.
¹ Also, for UX reasons, we'd want to print the shortest path between those two modules.
² Our main traversal is depth-first, so it's also not guaranteed to take the shortest possible path to each module.
As a result, there is an observable difference between (the import stacks printed by) initial and incremental builds.
Changelog:
TODO:
Differential Revision: D47548085