Bug 217613 – Web Inspector: Syntax highlighting for JSX is incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 411124 [details]
[Image] Bug

We started supporting JSX in 2018 (bug 181607). We rely on CodeMirror for syntax highlighting, perhaps we should update the JSX mode (External/CodeMirror/jsx.js).

[Radar WebKit Bug Importer]

<rdar://problem/70210975>

[Nikita Vasilyev]

I updated External/CodeMirror/jsx.js and it didn't solve the problem. In fact, the JSX mode of the current stable CodeMirror (5.58.1) is nearly identical to the one we use.

The problem is that https://reactjs.org/docs/create-a-new-react-app.html has JSX in App.js. Note the "js" extension, not "jsx". It's served with MIME-type text/javascript, too.

Web Inspector simply doesn't use JSX mode because it thinks it's a plain JS.

[Nikita Vasilyev]

Chrome DevTools handles JSX well and it also uses CodeMirror. Chrome seems to use JSX mode for all JS resources. This feels wrong but perhaps this is a lesser evil.

[Nikita Vasilyev]

Created attachment 417413 [details]
Patch

[Nikita Vasilyev]

Created attachment 417414 [details]
[Image] With patch applied

[Devin Rousso]

Comment on attachment 417413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417413&action=review

r-, see below

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:691
> +    CodeMirror.defineMIME(WI.mimeTypeForFileExtension("js"), "jsx");

I think CodeMirror only allows one mode per MIME type, which means that this would cause every `"text/javascript"` file to now be interpreted as JSX.  IMO, if the extension is `.js` and the MIME type is `text/javascript` then we really should be treating it as JavaScript, not JSX.

[Nikita Vasilyev]

Comment on attachment 417413 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417413&action=review

>> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:691
>> +    CodeMirror.defineMIME(WI.mimeTypeForFileExtension("js"), "jsx");
> 
> I think CodeMirror only allows one mode per MIME type, which means that this would cause every `"text/javascript"` file to now be interpreted as JSX.  IMO, if the extension is `.js` and the MIME type is `text/javascript` then we really should be treating it as JavaScript, not JSX.

This change shouldn't affect syntax highlighting of syntactically valid JS.
Besides, I don't know any better way of resolving this.

[Nikita Vasilyev]

Created attachment 417631 [details]
Patch

I changed the patch to only use JSX mode for source map JS resources.
Their MIME type is "synthetic"; it's determined from the resource extension already.

[Blaze Burg]

Comment on attachment 417631 [details]
Patch

I have no objections to this approach, with the precondition that it be tested on vanilla JS files to ensure it's not a pretty-printing regression. What testing has been performed with this change?

[Blaze Burg]

Comment on attachment 417631 [details]
Patch

I have no objections to this approach, with the precondition that it be tested on vanilla JS files to ensure it's not a pretty-printing regression. What testing has been performed with this change?

[Devin Rousso]

Can you verify that whether this interferes with pretty printing or other source map logic?

More generally though, I don't think this is a good path forward for a few reasons:
 - it's preferential towards react
 - it's incorrect (JSX !== JavaScript)
 - JSX is nonstandard and may clash in unexpected ways with future changes to JS
I'd be much less concerned with this being a default-off setting like "treat JS resources as JSX" or something, but even then I'm not sure this is tested/known enough not to cause issues with logic throughout Web Inspector.

[Greg Marriott]

Devin, do you have a concrete example of a well formatted JS file that fails when using JSX formatting?

[Blaze Burg]

Comment on attachment 417631 [details]
Patch

r=me

We need to move on, I don't think this is going to make pretty-printing source-mapped files any worse.

[EWS]

Committed r274230: <https://commits.webkit.org/r274230>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417631 [details].