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 code frames for errors for babel plugin #748

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

EvanBacon
Copy link
Contributor

What changed / motivation ?

Using code frame errors will show the dev generally where the problem is:

Additional Context

  • I'm not familiar with the repo. Wouldn't be offended if you close the PR and replicate with preferred style.

Screenshots, Tests, Anything Else

Screenshot 2024-10-14 at 7 31 59 PM

Pre-flight checklist

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 15, 2024
@nmn
Copy link
Contributor

nmn commented Oct 15, 2024

Thank you so much @EvanBacon

I was about to start working on this task myself, and your solution is honestly better than what I had in mind.

There is probably further improvements that can be made. Specifically, I'm working to expose the specific part of the code that is the problem for this, but this is already an improvement in the meantime.

Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can fix the Flow errors and failing tests then this looks good.

If you're not sure how to deal with Flow, I'm happy to do this for you!

@EvanBacon
Copy link
Contributor Author

@nmn I think you should drive this. I just took a guess at where the errors are but I'm still getting used to stylex so I'm not sure what the DX should feel like. Specifically we point to the general path object when things go wrong, which is already an improvement, but it's possible that we could assert on the precise dynamic variables that are triggering the errors.

Will continue to brush up on stylex and the repo though so I can be more helpful in the future.

@nmn
Copy link
Contributor

nmn commented Oct 16, 2024

@EvanBacon Thank you for putting this up and getting it so far! I'll drive it from here.

@nmn nmn merged commit f4c6ea7 into facebook:main Oct 16, 2024
7 of 8 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants