JSX Syntax highlighting broken?

[cozos]

Hey,

I was redirected here by support@github.com - it seems that JSX Highlighting is broken on Github.

For example: https://gist.github.com/cozos/d6636a93cba29c77e3221f2ca30212b0

import React from 'react';

var ExampleApplication = React.createClass({
  render: function() {
    var elapsed = Math.round(this.props.elapsed  / 100);
    var seconds = elapsed / 10 + (elapsed % 10 ? '' : '.0' );
    var message =
      'React has been successfully running for ' + seconds + ' seconds.';

    return <p>{message}</p>;
  }
});

var start = new Date().getTime();

setInterval(function() {
  ReactDOM.render(
    <ExampleApplication elapsed={new Date().getTime() - start} />,
    document.getElementById('container')
  );
}, 50);

If it helps, I'm on the latest build of Google Chrome.

[Alhadis]

Great. Would you mind patching this? I can't submit a PR because I'm blocked from the @atom org.

diff --git a/grammars/javascript.cson b/grammars/javascript.cson
index 469fb4b..fca11f9 100644
--- a/grammars/javascript.cson
+++ b/grammars/javascript.cson
@@ -1344,10 +1344,6 @@
           {
             'include': '#string_escapes'
           }
-          {
-            'match': "[^']*[^\\n\\r'\\\\]$"
-            'name': 'invalid.illegal.string.js'
-          }
         ]
       }
       {
@@ -1364,10 +1360,6 @@
           {
             'include': '#string_escapes'
           }
-          {
-            'match': '[^"]*[^\\n\\r"\\\\]$'
-            'name': 'invalid.illegal.string.js'
-          }
         ]
       }
       {

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___."). Thank you for your contributions.

[lildude]

I've opened https://github.com/atom/language-javascript/pull/642 with @Alhadis's suggested change.

[lildude]

@Alhadis I don't know if you can see https://github.com/atom/language-javascript/pull/642 but if not, the latest comment from @50Wliu for your changes is:

I thought about this for a bit: wouldn't this still make it so everything will be matched as a string until the next quote? I suppose that's better than angry red highlighting, but still not that ideal.

The answer would be "yes" unless you can think of a better solution.

[Alhadis]

I wasn't notified that I was mentioned at the atom/language-javascript repo... I guess being blocked org-wide affects that too. 👎

Yes, it would affect unclosed strings, but only affecting users who have Tree Sitter Parsers disabled in their settings:

While I've not tried this for myself, I believe users who leave this setting enabled by default will continue to see error highlighting for unclosed strings. I might be wrong though.

[Alhadis]

There's only one other possibility I can think of, one that would involve extra work on Atom (or GitHub's) end: add unclosed as a scope so the full scope-name becomes invalid.illegal.unclosed.string.js. Then ask the Primer team to modify their scope-to-CSS-class handling to avoid applying background colours to the .pl-ii class in JavaScript files only.

The only advantage this has over the previous solution is that Atom users who have Tree Sitter grammars turned off can continue to see error highlighting for unterminated strings. Which, given the complexity involved, probably isn't worth the effort.

[lildude]

I wasn't notified that I was mentioned at the atom/language-javascript repo... I guess being blocked org-wide affects that too. 👎

Thought as much 😞

Yes, it would affect unclosed strings, but only affecting users who have Text Mate Parsers disabled in their settings:

You mean tree sitter parsers 😉

Do you think there's an alternate solution for our case, ie TextMate, or is it very much an all or nothing approach that we're already proposing?

Answered as our updates crossed.

[Alhadis]

You mean tree sitter parsers 😉

Oops, yes, of course. 😀 What the hell.

Answered as our updates crossed.

I was following-up with a third comment saying "See above, our answers were posted at the same time", but I saw you notice and cross that sentence out. WebSockets are a wonderful thing, aren't they?

[lildude]

🎉 🤞 this finally puts this issue to 🛏 once and for all.

[Alhadis]

Awesome! 🎉 Though I had to check Graphemica.com to figure out what this meant:

[mythmon]

Unfortunately, it seems like this is still not fixed. I'm not sure if linguist has been updated with the new version of language-javascript, but I expected it would be after about two weeks. This PR still shows the red-highlighted false-positive unclosed strings.

[Alhadis]

@mythmon It's yet to take effect on GitHub. Changes to Linguist (including the grammars it uses for highlighting) only become live once a new release of Linguist has been cut, which happens roughly once a month.

[lildude]

This PR still shows the red-highlighted false-positive unclosed strings.

Not any more 😀

From a quick look over previously reported issues, all appear to be rendering correctly now so I'm considering this issue now resolved and closing. If a new instance of a problem comes to light, open a new issue.