Failing cases in JSX mode

[dabbott]

Following up on: #3742

Three from the jsx/index.html page:

1. Boolean attributes are not recognized properly (note the red self-closing />)
   
2. Block comments styled incorrectly (should be the brown comment color)
3. Line comments are not recognized properly (note the red self-closing />)
   

Two other things I've found:

4 . Elements should be allowed as attributes

5 . Indentation in JS expressions should be relative to the indentation level of JSX elements. I believe currently they get indented to prior JS indentation level. (this issue is highest priority, if I had to choose)

Current behavior gif here:

Correct position here:

Another example of current behavior (not followed by } char):

Correct position here:

@parisk @petetnt

[marijnh]

I pushed some patches that take care of most of these (though getting indentation right across language boundaries is always tricky, and I'm not sure if the result is what you consider the right way to indent these). But...

Elements should be allowed as attributes

What? I saw something like that in the grammar and assumed it was a typo. I guess we're no longer in XML Kansas anymore here, huh? What does this even do? That's going to be somewhat tricky to support since it does not correspond to the XML mode's expectations of what XML looks like.

[dabbott]

Awesome, thanks for fixing these so quickly! I'll test and confirm they're fixed. As far as style goes, I'm trusting what facebook shows on the reactjs site as the "proper" way to indent.

Yeah, I dunno, I've never seen anyone write elements as attributes in any JSX code I've ever seen in the wild over the past year... I believe it's exactly the same as writing foo={<Foo />}, except without the {}, for convenience. I don't personally find this case to be valuable or relevant, other than to properly adhere to the spec (which is not adhered to by Babel in a few places, as far as I can tell -- although Babel does transpile this case here).

It's in the spec as:

JSXAttributeValue :

" JSXDoubleStringCharactersopt "
' JSXSingleStringCharactersopt '
{ AssignmentExpression }
JSXElement

IMO, not worth the effort.

[petetnt]

I agree that direct JSXElements as props (such as <Foo bar=<Biz /> />) is a very low priority use case: like @dabbott, I have pretty much never seen them used like that, actually I didn't even know about it until I brainstormed with @lkcampbell about how CodeMirror could handle the props.

[dabbott]

1: Boolean attributes ✅

2 & 3: Comments are correct for the list of attributes <foo /*comment*/>, but in between an open and close tag they should be colored as regular text instead of comments <foo>/*not a comment*/</foo>. Transpiled correctly here (though colored wrong).

4: Won't fix, IMO.

5: Much much better. Still a few weird points. I'm not really sure how to systematically describe these other than as random one-offs of how they do/don't behave like I expect...

Fixed (although, should the closing } be electric?)

Better, but indents one spot too far, I think.

Good (also, closing } should be electric?)

Here I expect the closing } to align with showFooter.

Three more things I found!

6: Attribute indentation doesn't behave as I would expect. I would set multilineTagIndentPastTag: false on xml by default when jsx is used. This matches the attribute indentation style Facebook uses.

7: Fat arrow => expression doesn't parse correctly. If surrounded by () it's ok. Fat array => {statements} is also ok.

8: Very minor, but something's a little off about the demo page https://codemirror.net/mode/jsx/index.html. The tab character inserts the equivalent of 4 spaces, but adding a newline after a { or elsewhere only indents 2 spaces. I would be screenshotting the demo page instead of my own app, but I can't quite tell what's correct and what's not.

[marijnh]

The tab character inserts the equivalent of 4 spaces, but adding a newline after a { or elsewhere only indents 2 spaces.

This is CodeMirror's default behavior. You can configure it with the indentUnit and tabSize options if you want, but generally you'll want to use shift-tab (auto indent) rather than tab to indent lines.

I fixed comments only being comments inside of tags, and also added support for tag attributes (which, if you see them as a shorthand for {<tag/>}, aren't as hard to deal with as I initially thought). I've also disabled multilineTagIndentPastTag in the XML submode's configuration.

[dabbott]

1: ✅ Boolean attributes
2: ✅ Line comments
3: ✅ Block comments
4: ✅ Attr=<Element />. Great! Glad not to have to restrict what users can do.
5: JS {} indentation
6: ✅ Attr indentation
7: ✅ Fat arrow expressions
8: ✅ Not an issue, my bad

[dabbott]

This one's just a general javascript mode issue I found while testing jsx mode. Object destructuring with a trailing comma was throwing off the rest of the parsing:

Fixed in #3748:

It's here in the spec. (I thought maybe I was just writing weird code!)

13.3.3 Destructuring Binding Patterns

...

ObjectBindingPattern[Yield] :
{ }
{ BindingPropertyList[?Yield] }
{ BindingPropertyList[?Yield] , }

[dabbott]

I wonder if indentation is off because the open and close brackets are getting parsed in different modes? The { is styled as cm-m-xml, while the } is styled as cm-m-javascript. I only noticed this when I tried turning on some addons.

They don't quite match up with the edit/matchbrackets addon.

They sometimes get automatically closed with edit/closebrackets and sometimes don't.

[marijnh]

I wonder if indentation is off because the open and close brackets

The brackets are fed to the correct mode, but the way addModeClass works, it depends on innerMode returning the correcting thing before the token was read. I've updated the mode in 65950ad to address this.

[dabbott]

I tweaked the indentation within {} for the common cases. Indentation will look like this now:

I'm pretty much handling each case specially. I don't know if there's a better overarching rule that these follow that I haven't noticed.

One thing I wasn't sure how to handle:

I want to indent differently, depending on whether a <tag> is on the same line (and the start of the line) or not. I naively look at the previous token on this line for a > (here), but that's not quite right (since there can be junk in the way). Is there a better way to do this?

[marijnh]

Great. Anything left before we can close this?

[dabbott]

One last thing I found: currently the JS within the { } is getting parsed as a statement, when it should be parsed as an expression.

E.g. each property in this object is parsed as variable, and it's one long continued statement

Should look like this:

I took a stab at fixing by adding a hook

expectExpression: function(state) {
    state.cc.push(expressionNoComma)
}

to JS mode. Which mostly works, except that the curly braces are getting parsed by the JS mode also. So I tried:

1. Parsing the curly braces manually in JSX mode (and they end up cm-m-xml), so that expecting an expression works correctly. Which does work, but the logic is a little ugly, and I will have to mess with indentation a bit more, since now using the JS lexical.indented isn't quite right. https://github.com/codemirror/CodeMirror/compare/master...dabbott:parse-braces-in-jsx?expand=1

or

2. Do it all as a hook to JS mode, but the hook will be very specific to JSX. And I will probably have to pass in indent param and assign it to lexical.indented, or something, since this breaks indentation in an edge case. https://github.com/codemirror/CodeMirror/compare/master...dabbott:expect-expression-block?expand=1

In both cases I would still need to tweak indentation a bit more. Or maybe theres a better way entirely. What do you think?

[marijnh]

I think parsing the braces in the JSX mode is a sane approach (they aren't really part of the JS, anyway). You could set the JS mode to JSON to get it to parse the top-level as an expression, but that'll have some undesirable effect (such as turning off the comment info), so I think there it would make sense to extend the JS mode to also have a configuration that says 'expect an expression', without it being full-on JSON mode.

[julienw]

I can add another slightly broken example here:

  render() {
    this._scheduleDraw();
    return (
      <div className={this.props.className}>
        <canvas
          className={classNames(
            `${this.props.className}Canvas`,
            'threadStackGraphCanvas'
          )}
          ref={ref => (this._canvas = ref)}
          onMouseUp={this._onMouseUp}
        />
      </div>
    );
  }

And the result in current CodeMirror:

You can see some of the colors are still wrong: see the angle bracket in the ref's arrow function, or onMouseUp not having the right color, or the closing div tag.

[marijnh]

Pasting that code into http://codemirror.net/mode/jsx/ yields different results than what you screenshotted, and actually looks fine.

[julienw]

Ah right, I just used Codemirror's home page's demo, so likely was in HTML mode... Sorry about the disturbance...

Is there any drawback using JSX mode for all JS code ?

[marijnh]

JSX is a superset, so probably not.

[julienw]

Thanks ! Maybe it's not the right location to ask this, but why don't you merge JS and JSX modes, then ?

[adrianheine]

What do you mean by merging? If you're talking about code, mode/javascript/javascript.js already has 875 lines of code and defines JavaScript, TypeScript, JSON and JSONLD modes. It wouldn't make sense to put the ~150 lines for JSX in that file, too.

[julienw]

I don't really understand why, if there's already all this. I'm not saying the code should be physically copied, but enabling the Javascript syntax should IMO also enable JSX... especially if TypeScript and others are also already enabled.

[adrianheine]

If you select JavaScript mode, you get JavaScript, not TypeScript or JSX. These are different languages, they just happen to share a lot.