JSX indentation after fat-arrow attribute

[felipeochoa]

I just submitted a bug about this to bug-gnu-emacs@gnu.org, but there's a js2-specific piece to it. From the main bug report:

When indenting JSX code using js2- or js-mode, the indentation function gets confused when there's a fat arrow function in a JSX prop. Compare the way the following two code blocks are auto-indented:

const Component = props => ( // Incorrect indentation
    <FatArrow a={e => c}
      b={123}>
    </FatArrow>
);

const Component = props => ( // Correct indentation
    <NoFatArrow a={123}
                b={123}>
    </NoFatArrow>
);

One possible fix is to propertize the > in the fat arrow as punctuation, which would let sgml determine the correct end of the tag. Though there is an issue to resolve in js-mode allowing the use of parse-sexp-lookup-properties in the indentation logic delegated to sgml, if that is resolved, the following patch to js2-parse-function-internal would fix this issue:

@@ -8323,6 +8323,8 @@ Last token scanned is the close-curly for the function body."
           js2-loop-and-switch-set)
       (js2-parse-function-params function-type fn-node pos)
-      (when (eq function-type 'FUNCTION_ARROW)
-        (js2-must-match js2-ARROW "msg.bad.arrow.args"))
+      (and (eq function-type 'FUNCTION_ARROW)
+           (js2-must-match js2-ARROW "msg.bad.arrow.args")
+           (js2-record-text-property (1- (js2-current-token-end)) (js2-current-token-end) 'syntax-table
+                                     (string-to-syntax ".")))
       (if (and (>= js2-language-version 180)
                (/= (js2-peek-token) js2-LC))

If the upstream fix happens and you like this approach, happy to submit a PR for this + tests.

[dgutov]

Does the problem occur only in conjunction with JSX?

[felipeochoa]

Yes -- it's only when calling out to the sgml indent function. I suppose that means we could make the propertizing conditional on being in JSX mode

[dgutov]

With js2-mode only, I see broken indentation either way:

const Component = props => ( // Incorrect indentation
    <FatArrow a={e => c}
  b={123}>
    </FatArrow>
);

const Component = props => ( // Correct indentation
    <NoFatArrow a={123}
  b={123}>
    </NoFatArrow>
);

[felipeochoa]

Oh I see what you mean. I think js2-mode does not call out to sgml to handle xml indentation, so it is understandably confused. Fixing that would be a different issue altogether. But would that be in order to support E4X?

[dgutov]

I think js2-mode does not call out to sgml to handle xml indentation

js2-jsx-mode gives me the same results. But it should use sgml-indent-line when appropriate, it's the sole reason for its existence.

[felipeochoa]

Sorry have been super busy, but looking at this now with fresh eyes, I think I see we were talking past each other. Here's my assessment of the situation:

• rjsx-mode -- inherits indentation from js2-jsx-mode and thus from js-mode. For JSX elements, js-indent-line calls sgml-indent-line which gets confused by the fat arrow.
• js2-jsx-mode -- same story as rjsx-mode
• js2-mode -- It would be odd to edit JSX in vanilla js2, so it's extra confused 😄 (I don't think it calls out to sgml)

Fixing the whole thing requires going to js-mode (hence the upstream bug), but also requires that js2-mode's parser mark the fat arrow as punctuation.

If you are in vanilla js2-mode theoretically there wouldn't be any JSX, so this would not be an issue, and thus the text propertizing would be superfluous --- so a next iteration of the patch would make the recording conditional on having XML support.

[dgutov]

js2-jsx-mode -- same story as rjsx-mode

Okay, but when does the second example indent correctly for you? Like I said, js2-jsx-mode gives the bad indentation in both cases here.

Fixing the whole thing requires going to js-mode (hence the upstream bug)

For posterity, it's http://debbugs.gnu.org/24896.

so a next iteration of the patch would make the recording conditional on having XML support

Or on being inside an JSX literal, maybe?

[felipeochoa]

Okay, but when does the second example indent correctly for you? Like I said, js2-jsx-mode gives the bad indentation in both cases here.

Ah! A different bug. I'd tried the example without the comment in js2-jsx-mode and it worked just fine. Adding the comment results in this:

const Component = props => (  // Correct indentation
        <NoFatArrow a={123}
    b={123}>
        </NoFatArrow>
);

[metakermit]

Yup, I'm hitting this issue too, I think. When I'm doing a map with JSX syntax I'm getting:

render() {
    const messages = this.state.messages.map(
        message => <Message key={message.id}
        text={message.text}
        mine={message.mine} />
    );
    return messages;
}

I'd ideally expect something like:

render() {
    const messages = this.state.messages.map(
        message => <Message key={message.id}
                            text={message.text}
                            mine={message.mine} />
    );
    return messages;
}

It's even odder when I try to leave the argument to the arrow function in the line above:

render() {
    const messages = this.state.messages.map(message =>
                                             <Message key={message.timestamp}
                                             text={message.text}
                                             mine={message.mine} />
                                            );
    return messages;
}

For this second case, I'd hope to get:

render() {
    const messages = this.state.messages.map(message =>
        <Message key={message.timestamp}
                 text={message.text}
                 mine={message.mine} />
    );
    return messages;
}

(I get such results if I wrap the return expression in parentheses)

[aikeru]

Is this the same issue as reported here (and determined to be upstream)? If so I won't open another...
felipeochoa/rjsx-mode#34

[felipeochoa]

Same topic but different issue. There are a handful of indentation issues around, but I don't think this one has come up yet. I'd file it against js-mode, though

[metakermit]

@felipeochoa ok, I just sent an email about this to bug-gnu-emacs@gnu.org (at least how I understood you're supposed to report bugs related to js-mode). We'll see what they have to say about this…

[aikeru]

@metakermit any response/update from the mailing list? Did you find out if it was indeed the right place to submit js-mode bugs?

[metakermit]

@aikeru seems it was the right place, though I think it's not resolved yet. My bug report is visible here which someone merged to this bug which is still open from what I see. (having a hard time following the format of that issue tracking system 😄 )

[wyuenho]

Can you guys try this branch out and see if it's fixed? I think I've effectively by-passed @felipeochoa fix that's still pending to be merged into Emacs's js-mode. If you don't want to wait, I think my fix is pretty good for now.

[carloscheddar]

I'm having this issue as well I tried a few things but none of them solve the problem completely.

Fat arrow without parenthesis:

    return reviewers.map((reviewer, i) =>
                         <ReviewerTileContainer
                           key={`${reviewer.user}-${i}`}
                           className="col-md-4 col-xs-12"
                           >
                           <ReviewerTile reviewer={reviewer} />
                         </ReviewerTileContainer>
                        )

Fat arrow with parenthesis:

    return reviewers.map((reviewer, i) =>(
      <ReviewerTileContainer
        key={`${reviewer.user}-${i}`}
        className="col-md-4 col-xs-12"
        >
        <ReviewerTile reviewer={reviewer} />
      </ReviewerTileContainer>
    )
                        )

The second one is closer to what I'm expecting but the last parenthesis is trying to match the indentation of the matching parenthesis.

Map function in new line:

    return reviewers.map(
      (reviewer, i) =>(
        <ReviewerTileContainer
          key={`${reviewer.user}-${i}`}
          className="col-md-4 col-xs-12"
          >
          <ReviewerTile reviewer={reviewer} />
        </ReviewerTileContainer>
      )
    )

On this last one I managed to get the correct indentation(except for the closing > on ReviewerTileContainer but that seems to be fixed on @wyuenho branch) but I don't always want to add the map arguments on the next line.

Is there any fix for this?

[wyuenho]

js2-jsx-mode is basically the same as js-jsx-mode. js-jsx-mode relies on a lot of brittle heuristics to find the correct indentation for the current line. @felipeochoa has a pending patch for it sitting on Emacs' devs' mailboxes, but that just fixes arrow functions. Incorrect JSX indentation after {} is still an issue. Also, most likely you don't see it until at least Emacs 26.2 because it failed to apply to Emacs 26 before the code freeze.

As a temporary fix, you can use use-package-quelpa to pin rjsx-mode to my branch until it is merged. @felipeochoa will merge whenever he's got some time to finish reviewing it, right 😉 ?

[felipeochoa]

@wyuenho Yes! I'm sorry -- I meant to do it over the weekend, but couldn't find the time. Unfortunately not sure if this week will be any better. Might have to wait until next week ☹️

[wyuenho]

Haha no worries, it took me 2 weeks to write up those branches, I wouldn't blame you if it takes you longer to review them.