Replace JSX grammar

[pchaigno]

This pull request changes the JSX grammar to use https://github.com/babel/babel-sublime as discussed in #3779.

Description

Our current JSX grammar is locked to an old version because of a bug in the upstream grammar. With 632 commits behind the upstream to date, we're sure to miss important fixes and updates. Keeping the fork up-to-date is difficult because we have to watch out for PCRE incompatibilities in regular expressions.

This pull request changes the JSX grammar to use babel/babel-sublime. As discussed in #3779, the only issue is we have to change the scope to avoid a conflict with JavaScript.

@lildude I'm guessing Lightshow is still blocked by the security review... Any chance you could check on your side if the grammar applies correctly and if it fixes #3775?

Fixes #3775 (cf. example 1, example 2, example 3, and example 4).

• I am changing the source of a syntax highlighting grammar
  • Old: https://github-lightshow.herokuapp.com/?utf8=%E2%9C%93&scope=from-url&grammar_format=auto&grammar_url=https%3A%2F%2Fgithub.com%2Fgithub-linguist%2Flanguage-babel%2Fblob%2Fmaster%2Fgrammars%2FBabel%2520Language.json&grammar_text=&code_source=from-url&code_url=https%3A%2F%2Fgithub.com%2Fgithub%2Flinguist%2Fblob%2Fmaster%2Fsamples%2FJSX%2Fsample.jsx&code=
  • New: https://github-lightshow.herokuapp.com/?utf8=%E2%9C%93&scope=from-url&grammar_format=auto&grammar_url=https%3A%2F%2Fgithub.com%2Fgithub-linguist%2Fbabel-sublime%2Fblob%2Fmaster%2FJavaScript%2520%28Babel%29.tmLanguage&grammar_text=&code_source=from-url&code_url=https%3A%2F%2Fgithub.com%2Fgithub%2Flinguist%2Fblob%2Fmaster%2Fsamples%2FJSX%2Fsample.jsx&code=

[lildude]

I've taken a quick look at the difference in grammars and the babel/babel-sublime grammar produces quite different syntax highlighting for the simple example mentioned in #3775 (comment):

Interestingly, the current grammar doesn't show the problem either, but does render it quite differently:

The first example of those I reference in #3775 (comment) fails quite horribly with the babel/babel-sublime:

... vs our current grammar:

[pchaigno]

@lildude - @kivikakk mentioned the same highlighting issue in #4168, and it looks like it's coming from Lightshow. I'll wait for her go to update the original post with links to Lightshow.

[kivikakk]

@lildude @pchaigno As of now, this issue should be fixed. Here's the simple case rendering properly.

[pchaigno]

Thanks @kivikakk!

@lildude I've updated the original post with the appropriate links to Lightshow. Ready for review!

[lildude]

@pchaigno it certainly looks better, but still noticeably different:

Not sure if this is a grammar or Lightshow difference though and if it's significantly different that peeps will notice.

[Alhadis]

It's a grammar difference. If this grammar is anything like the language-babel grammar, then many of the scopes will differ to language-javascript. Most of the discrepancies are minor, but function calls in particular are being scoped strangely:

obj.methodName( arg ); /*
    ^^^^^^^^^^ - entity.name.function.js (Current)
               - variable.function.js (Babel grammar)
*/

Offending lines: #2375, #2408

Barring visual and aesthetic differences, the current scope-name (entity.name.function) is more correct according to the semantics of TextMate grammar naming conventions.

[pchaigno]

Yep, I've seen the missing colors for HTML tags but I thought it was worth it since it fixes the previous bugs? Maybe we can report the discrepancies upstream if they're not already.

[Alhadis]

I doubt upstream will be interested in accepting changes, since that's imposing the same unexpected (and apparently arbitrary) colour changes on users of the babel-sublime package. Part of why so many grammars continue to have incorrect scope-choices is because there's little incentive to justify the changes (beyond grammar-specific semantics few end-users care about).

Remember, these scopes dictate what users have to put in their stylesheets and preference files to tweak the colour choices...

[Alhadis]

@pchaigno It should be safe to perform a simple search-and-replace for the following patterns:

Search for:	Replace with:
<string>variable.function.js</string>	<string>entity.name.function.js</string>
<string>meta.tag.jsx entity.name.tag.jsx</string>	<string>entity.name.tag.html.jsx</string>

I believe including .html as a scope for JSX tags will adjust the highlighting to better match what we have currently, as that's the scope being used by our language-babel fork.

[pchaigno]

I doubt upstream will be interested in accepting changes, since that's imposing the same unexpected (and apparently arbitrary) colour changes on users of the babel-sublime package. Part of why so many grammars continue to have incorrect scope-choices is because there's little incentive to justify the changes (beyond grammar-specific semantics few end-users care about).

I was referring to the lack of HTML highlighting rather than scope names 😜

It should be safe to perform a simple search-and-replace for the following patterns:

Hm, is that something we could upstream?

[Alhadis]

Hold on, I just noticed there are spaces inside some of the scope names. Specifically, the ones affecting the HTML highlighting. Is that legal in XML TextMate grammars?

Because it's been known to screw with selector matching in Atom, so I wonder if it might be having an adverse effect on Lightshow too... :\

Hm, is that something we could upstream?

Nope. We're performing a mass change of dozens of scope names, which goes back to my previous point. 😉

[pchaigno]

Hold on, I just noticed there are spaces inside some of the scope names. Specifically, the ones affecting the HTML highlighting. Is that legal in XML TextMate grammars?

Looks like they're trying to define two scope names (separated by a white space) for the matched tokens... I'm surprised that even works with Sublime Text.

Nope. We're performing a mass change of dozens of scope names, which goes back to my previous point.

Right, my bad; I read a bit quickly.

[budmc29]

Any updates on this? I'm happy to help getting this merged.

[pchaigno]

I've updated the modified version of the babel-sublime grammar hosted at github-linguist/babel-sublime to change scope names as per @Alhadis' suggestions and to fix the double scope names. Here is the result:

Old is the previous grammar; New is the new grammar before the scope name changes; newer is the last version.

It's not perfect because we'll have to update the scope names if the grammar is updated, but at least it fixes #3775.

[Alhadis]

LGTM. 👍

[lildude]

Great work. Thanks. 🙇‍♂️

[peterbe]

Disclaimer; I haven't studied the details of this PR but I see that it was merged about a month ago and I'm still seeing busted display of JSX .js files. E.g. https://github.com/peterbe/whatsdeployed/pull/41/files

[lildude]

Disclaimer; I haven't studied the details of this PR but I see that it was merged about a month ago and I'm still seeing busted display of JSX .js files. E.g. https://github.com/peterbe/whatsdeployed/pull/41/files

That's probably because there hasn't been a Linguist release made since this PR was merged so you're still looking at the old behaviour. I aim to make a new release approximately monthly and today would make it exactly a month since the last release 😁

I'll try find time in the next week.

[mythmon]

As a follow up to the last comment, we're still seeing the same syntax highlighting problems on whatsdeployed PRs. This PR is extremely hard to review since most of the lines are covered in red backgrounds.

We've seen this problem pretty continuously over the last few months. I think it is a regression based on this change, and that nothing has happened in the last few months.

[lildude]

@mythmon This is happening because of the apostrophe right at the beginning of the JSX content and is essentially a continuation of #3044. We're attempting to get this fixed in the upstream grammar in #3044