Skip to content

Improve formatting of errors when building #11456

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

Merged
merged 5 commits into from
Nov 19, 2017
Merged

Conversation

misoguy
Copy link
Contributor

@misoguy misoguy commented Nov 4, 2017

Fix #11454
I don't know the history behind using error.codeFrame in catch()
but it seems that rollup is providing with the formatted lines inside error.frame instead of error.codeFrame
What I could find so far about rollup's error.frame is here
and the related getCodeFrame here

@gaearon
Copy link
Collaborator

gaearon commented Nov 4, 2017

Can you add a screenshot?

@misoguy
Copy link
Contributor Author

misoguy commented Nov 5, 2017

Sure this is the screenshot
image

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2017

It would be nice to get some syntax highlighting for this. Babel does it.

@misoguy
Copy link
Contributor Author

misoguy commented Nov 5, 2017

Could you add a screenshot of babel's syntax highlighting? To make sure we are on the same page :)

@misoguy
Copy link
Contributor Author

misoguy commented Nov 5, 2017

I'm guessing it's something like this?
image

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2017

Yes! And it would be nice to remove undefined from the header (when it's undefined).

@gaearon
Copy link
Collaborator

gaearon commented Nov 10, 2017

Do you plan to finish this with the syntax highlighting?

@misoguy
Copy link
Contributor Author

misoguy commented Nov 11, 2017

I would like to finish this with the syntax highlighting but there seems to be an issue with rollup's error object.
It seems that the error.frame used in this PR which has the error code frame without syntax highlighting also shows wrong line numbers.
The screenshot above shows that the error line is 13 while on the real file the error is on line 12.
I'm looking into rollup's issue regarding this problem but not sure when I can get a fix for it.

Would it be ok to leave this PR open in the mean time?
or should it be merged or closed for a new PR?

@gaearon
Copy link
Collaborator

gaearon commented Nov 11, 2017

Sure let's keep it open.

@misoguy
Copy link
Contributor Author

misoguy commented Nov 14, 2017

I have a PR open for the issue I mentioned about rollup's error object. rollup/rollup#1728
Hopefully it will be merged and released so that I can get back on track with the syntax highlighting 😄

@misoguy
Copy link
Contributor Author

misoguy commented Nov 16, 2017

@gaearon rollup has been released with my PR merged.
Therefore I have upgraded rollup dependency and added babel-code-frame to finish up with syntax highlighting properly as below 😄
image

@gaearon
Copy link
Collaborator

gaearon commented Nov 16, 2017

Can you please send a separate PR for updating Rollup first? Since it may not be obvious from from commit log and it's a substantial change.

Please also run yarn build to record build sizes before and after the change. (As separate commits.) This would tell us if Rollup update accidentally regresses on bundle sizes.

@misoguy
Copy link
Contributor Author

misoguy commented Nov 18, 2017

@gaearon I have sent a separate PR for updating rollup via #11591. I'll rebase this PR to the updated commit once it is merged.

@misoguy
Copy link
Contributor Author

misoguy commented Nov 18, 2017

Finished up this PR by removing rollup upgrade and merging master branch

@gaearon
Copy link
Collaborator

gaearon commented Nov 18, 2017

CI fails?

@misoguy
Copy link
Contributor Author

misoguy commented Nov 18, 2017

Didn't run yarn prettier 😞 Fixed it now.

@gaearon gaearon merged commit 962042f into facebook:master Nov 19, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 19, 2017

Thanks! This is great.

@gaearon
Copy link
Collaborator

gaearon commented Nov 27, 2017

It seems like there's an issue in case the error is a syntax error from Babel. Then error.loc.file is missing, and loc reports the wrong line number anyway.

@gaearon
Copy link
Collaborator

gaearon commented Nov 27, 2017

I fixed with 0182769.

Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* Improve formatting of errors when building

* Remove undefined from the header when error.plugin is undefined

* Add babel-code-frame and syntax highlighting in error message

* Run yarn prettier and fix code format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants