Adds filename to JSXTransform error messages #731
Conversation
vendor/browser-transforms.js
Outdated
There was a problem hiding this comment.
Could you show the erroneous line too? People often seem to get confused by line numbers when using inline script blocks (because lines count from the start of the script, not from the top of the file).
There was a problem hiding this comment.
This is already pointing the line number with e.lineNumber. Is this what you are asking for?
There was a problem hiding this comment.
No, I meant literally showing the problematic line of the file. (code.split('\n')[e.lineNumber] or something, maybe off by one, maybe also something needs to be done with \r.)
There was a problem hiding this comment.
Oh I see... but I'm not sure if we wanna do that... pointing to the file's erroneous line is good enough.
Also, as you can see from my screenshots, on Firefox and Chrome when you click on the file link it opens the file highlighting the erroneous line. Haven't tested on the other browsers but this should be good enough for development.
There was a problem hiding this comment.
As I said in my original message, people tend to get confused when using inline script blocks, because the line numbers won't match the file. Firefox and Chrome won't show the right line either in these cases.
There was a problem hiding this comment.
No line number will be shown for inline code, just when it comes from a file.
On inline code there is no source and it will just say that its coming from the current document (location.href).
I'm talking about the updated code, so lets try to discuss on the the new code.
Thank you for reviewing!
There was a problem hiding this comment.
@spicyj are we in sync or am I missing something?
There was a problem hiding this comment.
If you have an HTML page like:
1 <!DOCTYPE html>
2 <html>
3 <head><title>monkey</title></head>
4 <body>
5 <script src="JSXTransformer.js"></script>
6 <script type="text/jsx">
7 /** @jsx React.DOM */
8 var link = <a href={} />;
9 </script>
10 </body>
11 </html>then esprima will report an error on line 2 of the script, but that's line 8 of the original file. Since it's seemingly impractical to report the real line number, the next best alternative is to show the message along with the problematic line of code, along the lines of:
Uncaught Error: Parse Error: Line 2: XJS attributes must only be assigned a non-empty expression
at test.html, line 2
var link = <a href={} />;
^
There was a problem hiding this comment.
@spicyj I'll improve the error message for inline code, thank you.
Just to be clear, I already understood what you just said, the current patch doesn't show an incorrect line number. For inline code source will be undefined and the message will only say that the error is in the current document.
Right now it shows:
Uncaught Error: Parse Error: Line 2: XJS attributes must only be assigned a non-empty expression
at test.html
I'll improve it, thank you for pushing the improvement.
|
Please don't merge this yet, I'm thinking about improving this patch. I'll let you know when its fine. |
|
@spicyj here is how it looks after your feedback. |
|
Looks much better, thank you. Since (I believe) the common case will be lines of length ≤ 80, can we just show the entire line if it fits? |
|
We can't show a very long line (80 chars) because the bottom arrow (^) could be pointing to the wrong place since the line will be wrapped if the console width isn't long enough. |
|
Looks like there are merge conflicts, wanna rebase and then we can pull this in? |
|
@jeffmo merged it. |
Adds filename to JSXTransform error messages
Adds the filename to JSXTransform error message, making it easier to debug JSX syntax errors.
Fixes #393 and Fixes #394
This is how the error message look like:
Firefox
Chrome
