-
Notifications
You must be signed in to change notification settings - Fork 598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use shorter error message for file resolution issues. #1036
base: main
Are you sure you want to change the base?
Use shorter error message for file resolution issues. #1036
Conversation
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This looks great! Re: |
Oh, found the metro/packages/metro-resolver/src/resolve.js Lines 463 to 464 in c39b51b
We need to check that |
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
||
resolver = await createResolver({ | ||
resolver: { | ||
sourceExts: ['.fb.js', 'js', '.fb.json', 'json'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the case I was concerned about was something like ['.fb.js', 'js', 'json']
- where the fb
part should not be factored out, i.e. .(fb)?.(js|json)
would be wrong.
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: - split out of #1036 - Prevent `.native` extension on its own (in favor of `.native.js`, etc.). Pull Request resolved: #1045 Test Plan: - Updated the tests. Reviewed By: motiz88 Differential Revision: D47609008 Pulled By: robhogan fbshipit-source-id: c42ea7b78ef0ed7af6306c9b5264d770b211d776
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@EvanBacon Note that this still can't be merged, unfortunately, due to both merge conflicts with the mainline, and also some outstanding follow up still required (as requested by Moti). |
@rshest will follow up when I have more time. Wasn't accounting for the facebook-specific logic when I opened the PR. |
Summary
(/index)?
segment to prevent needing a second line.none of these files exist:
to account for the single lineno file matched the pattern
. I find this line more helpful as it implies the issue may be due to the pattern and not to the lack of file existence. Generally when users are reporting this issue to Expo CLI, it's because they need to modifysrcExts
.(.native|.native.js|.js|.native.json|.json)
->(.native)?.(native|js|json)
. This clearly has an issue where.native.native
would technically match the listed pattern––open to suggestions on how to format this differently. However,.native
seems like it shouldn't be present regardless. Worth noting that I hadn't noticed the extraneous.native
before because the length of the error message.Test plan