Bug 217613 - Web Inspector: Syntax highlighting for JSX is incorrect
Summary: Web Inspector: Syntax highlighting for JSX is incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-12 09:49 PDT by Nikita Vasilyev
Modified: 2021-03-10 12:43 PST (History)
6 users (show)

See Also:


Attachments
[Image] Bug (44.41 KB, image/png)
2020-10-12 09:49 PDT, Nikita Vasilyev
no flags Details
Patch (1.55 KB, patch)
2021-01-11 15:22 PST, Nikita Vasilyev
drousso: review-
Details | Formatted Diff | Diff
[Image] With patch applied (193.83 KB, image/png)
2021-01-11 15:24 PST, Nikita Vasilyev
no flags Details
Patch (1.97 KB, patch)
2021-01-14 09:20 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2020-10-12 09:49:22 PDT
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).
Comment 1 Radar WebKit Bug Importer 2020-10-12 09:49:34 PDT
<rdar://problem/70210975>
Comment 2 Nikita Vasilyev 2020-10-16 13:40:24 PDT
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.
Comment 3 Nikita Vasilyev 2020-10-16 16:25:16 PDT
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.
Comment 4 Nikita Vasilyev 2021-01-11 15:22:36 PST
Created attachment 417413 [details]
Patch
Comment 5 Nikita Vasilyev 2021-01-11 15:24:22 PST
Created attachment 417414 [details]
[Image] With patch applied
Comment 6 Devin Rousso 2021-01-11 15:57:10 PST
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.
Comment 7 Nikita Vasilyev 2021-01-11 16:01:50 PST
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.
Comment 8 Nikita Vasilyev 2021-01-14 09:20:11 PST
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.
Comment 9 BJ Burg 2021-01-19 16:04:46 PST
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?
Comment 10 BJ Burg 2021-01-19 16:04:47 PST
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?
Comment 11 Devin Rousso 2021-01-19 16:06:39 PST
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.
Comment 12 Greg Marriott 2021-01-21 00:55:03 PST
Devin, do you have a concrete example of a well formatted JS file that fails when using JSX formatting?
Comment 13 BJ Burg 2021-03-10 11:40:20 PST
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.
Comment 14 EWS 2021-03-10 12:43:41 PST
Committed r274230: <https://commits.webkit.org/r274230>

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