Remove invalid string highlighting from TextMate grammar

lildude commented 5 years ago

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
All new code requires tests to ensure against regressions

Description of the Change

GitHub's Linguist, which is used for syntax highlighting on GitHub.com, currently uses this grammar for highlighting Javascript and JSX. For quite some time now, users have experienced the problem where the presence of an apostrophe within JSX causes the syntax to be highlighted in red as an error:

@Alhadis proposed the solution of removing the "rule which highlights unterminated strings as errors" which @50Wliu suggested would probably be fine as the TextMate grammars are no longer actively maintained:

@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.

@50Wliu opened https://github.com/atom/language-javascript/issues/641 to track this. I'm now implementing the changes suggested by @Alhadis in this PR.

This change only affects the TextMate grammar as this is the grammar Linguist uses as tree-sitter grammars are not currently supported.

Alternate Designs

Benefits

Syntax highlighting on GitHub.com for Javascript with JSX that contains apostrophes will be rendered correctly.

Possible Drawbacks

As this change only affects the TextMate grammar, there is a risk that the editor behaviour may vary slightly from the representation on GitHub.com for as long as we use the TextMate grammar.

Applicable Issues

See https://github.com/github/linguist/issues/3044 for further details of the problem we're trying to resolve.

Fixes https://github.com/atom/language-javascript/issues/641

lildude commented 5 years ago

🤔 Sorting out the tests is going to be interesting as we only want these changes to be considered when testing the TextMate grammar. I'm open to suggestions on how best to do this.

lildude commented 5 years ago

Ooo, maybe not. If I'm reading the tests correctly, the tests only run against the TextMate grammar:

https://github.com/atom/language-javascript/blob/e5034e28f00e5401890516b7bce47fe3a9041c3c/spec/javascript-spec.coffee#L14-L15

So whatever test changes I make, they'll only apply to the testing with the TextMate grammar.

winstliu commented 5 years ago

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.

lildude commented 5 years ago

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?

Yup. I've gone back to @Alhadis to see if he's got a better idea or implementation method as I agree, it's not ideal, however it's certainly waaaay better than all the red... like in this PR https://github.com/peterbe/whatsdeployed/pull/81/files 😭

lildude commented 5 years ago

Heard back:

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?

Yup, but this would only affect GitHub and Atom users that have the treesitter grammars disabled. An alternate solution is waaay too complex and requires changes to Atom.

winstliu commented 5 years ago

All right, let's do this then.

winstliu commented 5 years ago

🚀 as language-javascript@0.129.21.

atom / language-javascript