Skip to content

fix: skip errors that occurred on virtual lines - #1226

Open
EvanBacon wants to merge 4 commits into
react:mainfrom
EvanBacon:@evanbacon/metro/fix-errors-in-babel-code
Open

fix: skip errors that occurred on virtual lines#1226
EvanBacon wants to merge 4 commits into
react:mainfrom
EvanBacon:@evanbacon/metro/fix-errors-in-babel-code

Conversation

@EvanBacon

Copy link
Copy Markdown
Contributor

Summary

If you have a module that has import statements generated in the transformer, then it could possibly throw a resolver error on a line that won't exist in the on-disk file. Without this change, the process will throw an unhandled error. Here, we just skip the error improvement step and fallback on the default behavior.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 21, 2024
@motiz88

motiz88 commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

Where is the bad source location coming from? This kinda looks like it masks a genuine bug elsewhere. Do you have repro steps? Also, we have tests for this code now, might be worth adding a regression test to cover the fix.

@EvanBacon

Copy link
Copy Markdown
Contributor Author

@motiz88 If you have a babel plugin which takes an empty file like "" and injects a bunch of imports:

import "missing"
import "missing2"

Then the resolver will throw an error which breaks the formatter because it attempts to read lines that don't exist on-disk.

EvanBacon and others added 3 commits March 5, 2024 15:20
Brings the branch up to react/metro main at 0ff28e6. No conflicts; the branch's only difference from main remains the five-line guard in refineDependencyLocation.
This fix has had no test since the PR was opened, which is what the review asked for in 2024. Adds one at the integration level, in `build-errors-test.js`, so it exercises the real transform and resolution path rather than constructing the error directly.

`injectImportPastEndOfFileTransformer.js` wraps `@react-native/metro-babel-transformer` and, for one fixture, appends `import './does-not-exist';` parsed with `startLine` one line past the end of the source — which is what a plugin that generates and appends code does. The dependency Metro collects then reports a location with no counterpart in the file on disk, so building the code frame for the resolution error walks off the end of `lines`.

Without the guard, the build rejects with `TypeError: Cannot read properties of undefined (reading 'length')` in place of the resolution error. With it, the error survives and the code frame degrades to the lines that do exist.

The assertion is deliberately not a snapshot: the resolver's list of candidate paths varies between versions, and what matters here is only that the resolution error survives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants