Indentation Fixes #75

wyuenho · 2018-05-16T03:10:25Z

~~# Merge after #72~~

~~## Diff~~

Fixes:

js2-jsx-mode - Issue with intendation after code block in jsx mooz/js2-mode#490
js2-jsx-mode - controlling indent for the closing part of a self-closing tag mooz/js2-mode#482
js2-jsx-mode indent bug when nesting jsx in render={} attribute mooz/js2-mode#462
Strange indentation when when wrapping return statement in brackets mooz/js2-mode#451

Similar to js-jsx-indent-line, but fixes indentation bug with JSXElement after a JSX expression and arrow function. In addition, the > of a multi-line open tag and the /> of a multi-line self-closing tag are aligned to the beginning <. This function also ensures everything inside a JSX context is indented according to js-indent-level using spaces, this is due to the limitation of sgml-indent-line not being able to indent with tabs.

As an added bonus, this also fixes the rjsx-indentation-1 test for Emacs < 26.

floscr · 2018-06-26T16:31:19Z

Just wanted to say that this PR works wonderful 👍
Thanks for the work!

bryzettler · 2018-08-15T18:17:13Z

This beast ready to be merged now that #72 is?
And maybe get a release pushed up to melpa?

felipeochoa · 2018-08-15T18:18:27Z

Yup -- was planning to rebase later today to give it a final look and merge

wyuenho · 2018-08-16T00:09:31Z

~~I've rebased the branch now.~~

There's some problems with this, will deal with it later tonight

felipeochoa · 2018-08-16T16:21:20Z

Ok, thanks! Let me know when it's ready. I have a bit more time now for open source :)

wyuenho · 2018-08-17T00:22:19Z

@felipeochoa I've backed out the docs you've put in this branch, they still exist on your fork in a branch, you can reapply them after this PR is merged. This PR should be good to go.

felipeochoa · 2018-08-17T15:31:02Z

Ok, I think I understand how this is working, and I like it! I have a couple of nits, and a couple of substantive questions. Happy to handle the nits myself if you're busy, and also happy to leave the substantive Qs as bugs/todo, but want to understand nonetheless

felipeochoa · 2018-08-17T15:54:31Z

One other high-level question: does this code handle the case of a top-level tag with its > or /> on a new line? I only ask because it looks like the main cond in rjsx-indent-line exits early for top-level tags.

Thanks again for pulling this together!!

wyuenho · 2018-08-17T21:57:21Z

Happy to handle the nits myself if you're busy, and also happy to leave the substantive Qs as bugs/todo, but want to understand nonetheless

Sounds good, but have you reviewed the PR but forgot to submit the review again?

does this code handle the case of a top-level tag with its > or /> on a new line?

No. Top-level JSX is a noop, there's no point dealing with this edge case. The same approach is used for commenting as well.

felipeochoa · 2018-08-17T15:32:34Z

rjsx-mode.el

+
+(defvar-local rjsx--indent-line-columns nil
+  "A list of indent columns on a per-line basis. The car is the
+  indent column of the last line, cadr the indent column of the


[nit] Docstring spacing is off here and a couple of other places

felipeochoa · 2018-08-17T15:33:42Z

rjsx-mode.el

+limitation of `sgml-indent-line' not being able to indent with
+tabs.
+
+Fixes:


Hopefully these will be fixed upstream at some point (!). Maybe better to leave this as a comment than in the docstring?

Nah. The only way to fix this upstream satisfactorily is to merge js2-mode and rjsx-mode. Text-based heuristics is a thin PVC pipe under 10 tons of pressure, it doesn't matter how to you plug it, it'll leak.

felipeochoa · 2018-08-17T15:36:50Z

rjsx-mode.el

+  (let* ((sgml-basic-offset js-indent-level) ;; `sgml-indent-line' can only indent by spaces
+         (cur-pos (save-excursion (back-to-indentation) (point)))
+         (cur-char (char-after cur-pos))
+         (node (js2-node-at-point (- cur-pos rjsx--indent-running-offset))))


I'm trying to understand this rjsx--indent-running-offset bit. Is the idea to simply convert between "current" buffer coordinates and "as of last parse" buffer coordinate? If so, a defsubst for this would be great. There's a comment further down where I think we need this but don't have it.

It's an optimization to prevent having to reparse to get the node of the beginning of the line after indenting a line.

After indenting a line, the "offset" is the difference of the position of the beginning of the line between "before" and "after" indentation. If you are indenting a region, the offsets accumulate in a list of +4 -5 +2..... etc on a line by line basis. Adding them all together and subtracting the current position will get you back the position of the node you need to process, without reparsing the whole file.

felipeochoa · 2018-08-17T15:39:39Z

rjsx-mode.el

+      (cond
+       ;; Top-level tag
+       ((not (rjsx-node-p (js2-node-parent node)))
+        (cl-letf (((symbol-function #'js--continued-expression-p) 'ignore))


[nit] I get why you're doing this, but cl-letfing a private function is usually worth a quick explanatory comment. E.g.,

;; Prevent `js-indent-line' from treating the leading `<' as a continued expression

felipeochoa · 2018-08-17T15:41:16Z

rjsx-mode.el

+          (rjsx-indent-with-spaces cur-pos (js-indent-line))))
+       ;; Align the /> of a multi-line self-closing tag
+       ((and (not (rjsx-node-closing-tag node))
+             (save-excursion (goto-char cur-pos) (looking-at "/[[:space:]]*>")))


[nit] Use `looking-at-p' here.

felipeochoa · 2018-08-17T15:44:22Z

rjsx-mode.el

+       ((and (rjsx-node-p (js2-node-parent node))
+             (char-equal ?> cur-char))
+        (rjsx-indent-with-spaces cur-pos (js-indent-line)))
+       ;; Get around `js-indent-line' bug with not aligning JSXElement sibling


[nit] Is there a bug number to go with this workaround? One of the ones above?

This just explains the root cause for one of those in the docstring, I don't remember which one tho.

felipeochoa · 2018-08-17T15:45:26Z

rjsx-mode.el

+       ;; Get around `js-indent-line' bug with not aligning JSXElement sibling
+       ;; after a JSX expression.
+       ((rjsx-wrapped-expr-p
+         (let* ((parent (js2-node-parent node))


[nit] I suspect this could be simplified to avoid looping twice, but for now can you just factor it out into a helper function? It's a bit dense, and I think putting a name + docstring would make it much easier to understand what's going on. (I think it's just looking for the preceding non-blank child?)

I suspect this could be simplified to avoid looping twice

O(2 * 1) = O(1 * 2), meh

I think it's just looking for the preceding non-blank child?

That's right. I'd prefer not to make a function that's just used once. I'll put a comment here.

felipeochoa · 2018-08-17T15:45:56Z

rjsx-mode.el

+       ;; after a JSX expression.
+       ((rjsx-wrapped-expr-p
+         (let* ((parent (js2-node-parent node))
+                (children (cl-loop for kid in (cl-struct-slot-value 'rjsx-node 'kids parent)


[nit] isn't there an accessor we could use here?

felipeochoa · 2018-08-17T15:46:41Z

rjsx-mode.el

+      (rjsx-indent-with-spaces cur-pos (js--as-sgml (sgml-indent-line))))
+     ;; Inside JSX expression or looking at the end of it
+     ((rjsx-ancestor node #'rjsx-wrapped-expr-p)
+      (if (cl-loop for c being the elements of "[])},;" thereis (char-equal c cur-char))


[nit] (memq cur-char '(?\[ ?\] ?\) ?\} ?, ?\;)) is easier to read (for me) and much faster

felipeochoa · 2018-08-17T15:47:17Z

rjsx-mode.el

+         (progn
+           (setq rjsx--indent-region-p t)
+           (js2-indent-region start end))
+       (setq rjsx--indent-running-offset 0


[nit] Are these used anywhere else? If not, why not just let-bind them around the js2-indent-region' call and get rid of the unwind-protect' form?

felipeochoa · 2018-08-17T22:01:13Z

Oops. Submitted now. Makes sense re: top-level. I think I actually understand this one 😄

wyuenho · 2018-08-18T00:08:15Z

I've left the docstring alone but cleaned up the rest. Thanks for reviewing!

…-mode#490

felipeochoa · 2018-08-18T16:58:51Z

@wyuenho Thanks again for all your work on rjsx!!

Related to #75

wyuenho changed the title ~~Indent after JSX Expression~~ Indentation Fixes May 16, 2018

ztlevi mentioned this pull request May 17, 2018

Use rjsx-mode for react framework syl20bnr/spacemacs#10627

Closed

wyuenho mentioned this pull request Jun 26, 2018

Fix #46 Support JSX comments #72

Merged

felipeochoa reviewed Aug 17, 2018

View reviewed changes

Fix mooz/js2-mode#482, mooz/js2-mode#462, mooz/js2-mode#451, mooz/js2…

b211a8c

…-mode#490

felipeochoa merged commit bc62efe into felipeochoa:master Aug 18, 2018

wyuenho deleted the indent-after-jsx-expr branch August 18, 2018 17:28

felipeochoa added a commit that referenced this pull request Aug 21, 2018

Update README now that we add some indentation

600d878

Related to #75

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indentation Fixes #75

Indentation Fixes #75

wyuenho commented May 16, 2018 •

edited

Loading

floscr commented Jun 26, 2018

bryzettler commented Aug 15, 2018 •

edited

Loading

felipeochoa commented Aug 15, 2018

wyuenho commented Aug 16, 2018 •

edited

Loading

felipeochoa commented Aug 16, 2018

wyuenho commented Aug 17, 2018

felipeochoa commented Aug 17, 2018

felipeochoa commented Aug 17, 2018

wyuenho commented Aug 17, 2018 •

edited

Loading

felipeochoa Aug 17, 2018

felipeochoa Aug 17, 2018

wyuenho Aug 17, 2018

felipeochoa Aug 17, 2018

wyuenho Aug 17, 2018 •

edited

Loading

felipeochoa Aug 17, 2018

felipeochoa Aug 17, 2018

felipeochoa Aug 17, 2018

wyuenho Aug 17, 2018

felipeochoa Aug 17, 2018

wyuenho Aug 18, 2018

felipeochoa Aug 17, 2018

felipeochoa Aug 17, 2018

felipeochoa Aug 17, 2018

felipeochoa commented Aug 17, 2018

wyuenho commented Aug 18, 2018

felipeochoa commented Aug 18, 2018

Indentation Fixes #75

Indentation Fixes #75

Conversation

wyuenho commented May 16, 2018 • edited Loading

floscr commented Jun 26, 2018

bryzettler commented Aug 15, 2018 • edited Loading

felipeochoa commented Aug 15, 2018

wyuenho commented Aug 16, 2018 • edited Loading

felipeochoa commented Aug 16, 2018

wyuenho commented Aug 17, 2018

felipeochoa commented Aug 17, 2018

felipeochoa commented Aug 17, 2018

wyuenho commented Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyuenho Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeochoa commented Aug 17, 2018

wyuenho commented Aug 18, 2018

felipeochoa commented Aug 18, 2018

wyuenho commented May 16, 2018 •

edited

Loading

bryzettler commented Aug 15, 2018 •

edited

Loading

wyuenho commented Aug 16, 2018 •

edited

Loading

wyuenho commented Aug 17, 2018 •

edited

Loading

wyuenho Aug 17, 2018 •

edited

Loading