Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EvanBacon
Copy link
Contributor

Summary

  • Part of the resolution error is very verbose and somewhat hard to make sense of.
  • This PR condenses the matchable segments to reduce duplicate contents.
    • Add optional (/index)? segment to prevent needing a second line.
    • Modify header none of these files exist: to account for the single line no 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 modify srcExts.
    • Reduce duplicate references in extensions (.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.
  • reference to discord proposal.
Screenshot 2023-07-15 at 6 32 54 PM

Test plan

  • Unit tests cover this well.

@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 Jul 15, 2023
@facebook-github-bot
Copy link
Contributor

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@motiz88
Copy link
Contributor

motiz88 commented Jul 17, 2023

This looks great! Re: .native.native, that seems weird - is it just an artifact of the current tests or does it show up in real-world usage too?

@motiz88
Copy link
Contributor

motiz88 commented Jul 17, 2023

Oh, found the .native bug:

if (context.preferNativePlatform) {
const filePath = resolveSourceFileForExt(context, `.native${sourceExt}`);

We need to check that sourceExt isn't empty.

@facebook-github-bot
Copy link
Contributor

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/metro-resolver/src/resolve.js Show resolved Hide resolved

resolver = await createResolver({
resolver: {
sourceExts: ['.fb.js', 'js', '.fb.json', 'json'],
Copy link
Contributor

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.

@facebook-github-bot
Copy link
Contributor

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Jul 20, 2023
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
@facebook-github-bot
Copy link
Contributor

@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rshest
Copy link
Contributor

rshest commented Jul 31, 2023

@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).

@EvanBacon
Copy link
Contributor Author

@rshest will follow up when I have more time. Wasn't accounting for the facebook-specific logic when I opened the PR.

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.

None yet

4 participants