-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
JSX Syntax highlighting broken? #3044
Comments
Linguist only selects the grammar to use for highlighting. For JSX, GitHub uses gandm/language-babel. You should open an issue there if there isn't already one. Sorry for the two redirects :S |
👌 |
Closing as this is an upstream issue. |
@arfon @pchaigno Author of language-babel here. I've just checked my grammar using the version on linguist - 2.25.1 - inside Atom and it appears to work so I don't know what is going on. but on github /***
XMLHttpRequest example
*/
import 'js/web.jsx';
import 'console.jsx';
class _Main {
static function main(args : string[]) : void {
var xhr = new XMLHttpRequest();
var path = dom.document.location.pathname;
xhr.open("GET", path.replace(/\/[^\/]*$/, "") + "/hello.txt");
xhr.onreadystatechange = function (e) {
if (xhr.readyState == xhr.DONE) {
_Main.update(xhr.responseText);
}
};
xhr.onerror = function (e) {
console.error("XHR error");
};
xhr.send();
}
static function update(text : string) : void {
var output = dom.id("output");
var textNode = dom.document.createTextNode(text);
output.appendChild(textNode);
}
}
// vim: set expandtab: |
@gandm Not 100% sure if this is the issue, but your Also, I couldn't help noticing you've escaped semicolons in your expressions ( |
I just ran it through RegexBuddy and it's OK. |
\s_(?:(?:(\bfrom\b)?+\s++(('|")([^\"']_)(\k<-3>)))|(?=;|^\s*\b(if|switch|try|var|let|const|static|function|return|class|do|for
Options: Case sensitive; Exact spacing; Dot doesn’t match line breaks
Created with RegexBuddy |
What exactly are we trying to prove? Of course I would expect that end statement to match |
I said I wasn't sure if it was the cause of the problem. But I believe I've found an issue @gandm can investigate: there seems to be an issue somewhere in the Since the first |
I've just run code that works in oniguruma but does nothing in lightshow. I use this type of back referencing a lot in the grammar. Maybe this is the issue? Test Grammar
against code
should highlight in reverse red. Seems like backreferencing isn't working in whatever Regex engine is used by Lightshow but does in Oniguruma engine as used by Atom. |
That was my theory, but it still wasn't matching when I replaced it with an ordinary backreference. |
Yeah I think back referencing is broken in whatever engine is used in lightshow and I guess by extension in linguist. What engine is it? |
I don't know, nobody outside GitHub does. The damn thing is closed source. |
Well I could fix this single issue by changing the end for imports regex but the grammar uses this in other places where it isn't possible to change it. Therefore it may be prudent to look at other grammars that parse jsx but don't use back refererencing and use one of these inside linguist. |
Nope, it's a separate issue. Basically, Lightshow seems to be powered with a different regex engine that only supports a subset of Oniguruma's extensions. I say "subset" because most TextMate grammars already use Oniguruma extensions in the form of |
OK thanks. |
I just noticed that @cozos, who raised the issue, appears to be using jsx to mean the statically typed OO language JSX and not the JSX extension to JavaScript as used by facebook's React. Maybe linguist has better support for this language using another suffix that he could use? It makes no difference to the underlying issue, as React JSX would also get this problem due to the incompatibility between the linguist regex engine and Atom's Oniguruma. |
@gandm I apologize, I actually meant the JSX extension to JavaScript used by Facebook's React - I was just looking for some examples of JSX on Github and seeing that the syntax highlighting issue was also happening on the statically typed JSX I carelessly assumed it was the React JSX. I will edit the example to this: 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); |
Happy to see this issue getting some discussion, but I'm not sure where it landed... Was this reopened because it's a bug linguist is going to fix? |
@spalger I would also like to mention to @arfon and @Alhadis that JSX syntax highlighting was working on Github probably around a month and a half ago - so perhaps there was some changes on linguist's end? |
@spalger @cozos it would have worked about a month ago but then I refactored the import/export part of the grammar to use a certain back reference notation that oniguruma supports but linguist doesn't appear to. Actually, I've always used this form of back reference in other parts of the grammar that handled flow syntax but I guess no one has picked up on it not working as flow isn't widely used. Maybe I could change the grammar to support something that linguist can use but I'm reluctant on two counts. One, it makes the code less readable and more unique for each regex, and secondly, I believe linguist should be text mate compatible and it isn't. |
Why doesn't linguist just use https://github.com/gandm/language-babel/releases/tag/v2.24.4 then? @gandm, |
Unfortunately, I believe this is still an issue. A single apostrophe was the cause for me. This stackoverflow post helped me figure it out. As you can see in the commit below the When I changed the file extension to I changed the file extension back to I hope this was helpful. |
This is going to be a problem with the grammar itself and not Linguist. You're using the I know @Alhadis has been working on a grammar to combine support for both of these into the same grammar though I'm not sure of the status. |
Ditch Atom's grammar, and switch to Babel's until I can get something battle-ready. There'll be no visible discrepancies between JS and JSX files (caused by different scope choices in either grammar), and all valid JavaScript can be guaranteed to be valid JSX. |
Hmm, this seems easier said than done. A metric 🚣♂️ load of other grammars expect to find
Seems the grammar compiler isn't pulling in all of the files in the grammar so isn't finding the diff --git a/grammars.yml b/grammars.yml
index 0ff1dcd9..73b18475 100755
--- a/grammars.yml
+++ b/grammars.yml
@@ -448,11 +448,6 @@ vendor/grammars/language-html:
- text.html.basic
vendor/grammars/language-inform7:
- source.inform7
-vendor/grammars/language-javascript:
-- source.js
-- source.js.regexp
-- source.js.regexp.replacement
-- source.jsdoc
vendor/grammars/language-jison:
- source.jison
- source.jisonlex The removal of $ script/add-grammar --replace language-javascript https://github.com/github-linguist/babel-sublime
Checking docker is installed and running
$ docker ps
Deregistering: vendor/grammars/language-javascript
$ git submodule deinit vendor/grammars/language-javascript
$ git rm -rf vendor/grammars/language-javascript
$ script/grammar-compiler update -f
Registering new submodule: vendor/grammars/babel-sublime
$ git submodule add -f https://github.com/github-linguist/babel-sublime vendor/grammars/babel-sublime
$ script/grammar-compiler add vendor/grammars/babel-sublime
> latest: Pulling from linguist/grammar-compiler
> Digest: sha256:159ff655c832de0e3ab3ea1366992606b5f604bcb64f8757b6c575d4a7628bc6
> Status: Image is up to date for linguist/grammar-compiler:latest
> The new grammar repository `vendor/grammars/babel-sublime` (from https://github.com/github-linguist/babel-sublime) contains 1 errors:
> - Missing include in grammar: `source.js.jsx` (in `JavaScript (Babel).tmLanguage`) attempts to include `source.regexp.js` but the scope cannot be found
>
> failed to compile the given grammar
$ |
Ah heck, that's bad. 😓 Didn't expect there'd be a circular dependency involved... that complicates everything. @50Wliu, any idea if the Atom team would be averse to removing the rule which highlights unterminated strings as errors? Since Atom's grammars currently ship with both TextMate and Tree-sitter grammars (the latter of which Linguist ignores), I imagine the impact to users is minimal… |
@Alhadis that's probably fine. We don't actively maintain the TextMate variants anymore, but I think this is something we'll merge a PR for. |
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'
- }
]
}
{ |
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. |
I've opened atom/language-javascript#642 with @Alhadis's suggested change. |
@Alhadis I don't know if you can see atom/language-javascript#642 but if not, the latest comment from @50Wliu for your changes is:
The answer would be "yes" unless you can think of a better solution. |
I wasn't notified that I was mentioned at the 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. |
There's only one other possibility I can think of, one that would involve extra work on Atom (or GitHub's) end: add 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. |
Thought as much 😞
You mean tree sitter parsers 😉
Answered as our updates crossed. |
Awesome! 🎉 Though I had to check Graphemica.com to figure out what this meant: |
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. |
@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. |
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. |
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
If it helps, I'm on the latest build of Google Chrome.
The text was updated successfully, but these errors were encountered: