Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indentation with tabs does not work correctly #85

Open
matthemsteger opened this issue Sep 22, 2018 · 10 comments · May be fixed by #109
Open

Indentation with tabs does not work correctly #85

matthemsteger opened this issue Sep 22, 2018 · 10 comments · May be fixed by #109

Comments

@matthemsteger
Copy link

matthemsteger commented Sep 22, 2018

After the fix in 68fe4c0 it looks like tabs are replaced by (the appropriate number of) spaces. I can dig a little further, but this appeared after this fix (which I am thankful for, it made writing JSX very difficult).

Switching to js2-mode makes things work appropriately.

Various indentation variables (I use editorconfig-emacs to set these)
js2-basic-offset: 4
js-indent-level: 4
sgml-basic-offset: 4
indent-tabs-mode: t

@felipeochoa
Copy link
Owner

This is a known limitation of the indentation code. I would have thought the change happened in eae8137, though. I should add this to the README, but for now if you want tabs you can revert to the native js-mode indentation, you can use (setq-local indent-line-function 'js-jsx-indent-line) in your rjsx-mode-hook or similar.

As always, PRs welcome. I don't use tabs myself, so this is unlikely to get fixed soon. (Since I'm not familiar with how indent-tabs-mode should behave, it would mean a lot of manual reading for me to even know what the goal is).

That said, if anyone's interested, I would start by removing this binding and seeing what happened. It might be that that alone is enough.

Otherwise, I'd look into rjsx--indent-line-to-offset and its callsites, since that handles all the JSX-specific indentation. The JS indentation is delegated to js-indent-line, which it sounds like already handles tabs correctly. I think the indentation code is straightforward (basically just a switch on the various JSX pieces), but happy to help interpret if necessary.

@matthemsteger
Copy link
Author

Ahhh, yeah, I wasnt 100% sure on the timing because at my job we use spaces and at home I prefer tabs (its a constant internal struggle).

At any rate, I will try the workaround. I wish I could quickly submit a PR, but unfortunately elisp is very alien to me so it will take some getting up to speed.

@futpib
Copy link

futpib commented Apr 4, 2019

A workaround alternative to what README suggests:

(add-hook 'rjsx-mode-hook
          (lambda ()
            (add-hook 'before-save-hook
                      (lambda ()
                        (if indent-tabs-mode
                            (tabify (point-min) (point-max))
                          (untabify (point-min) (point-max))
                          ))
                      nil
                      t)))

@matthemsteger
Copy link
Author

So I had been using this for quite a while, but after the update to Emacs 27, this work around no longer works because of mooz/js2-mode#529. I tried to see if I could create a new major mode locally that derived from rjsx-mode that set the things that were trying to be set in mooz/js2-mode#523, but that did not work with a lot of the nice code in in rjsx-mode to make writing JSX better.

So unfortunately, not a great solution now for tabs.

  1. Use js-mode and js2-minor-mode and live with worse syntax highlighting and no extra JSX features, but correct indentation
  2. Use rjsx-mode and have no indentation really, and rely on something else (like prettier) to just clean up your code

Right now I am doing the second.

I tried to modify rjsx-mode by implementing the change in #109 but that did not work. I suspect it probably worked prior to Emacs 27 and the rewrite of everything in js-mode.

@matthemsteger matthemsteger changed the title After 68fe4c0 indentation fix tabs do not work correctly Indentation with tabs does not work correctly Dec 12, 2020
@dgutov
Copy link

dgutov commented Jan 17, 2021

@matthemsteger So what happens if you just remove that indent-tabs-mode binding from the code like Felipe's suggested in the first comment here?

Like thornjad@e0a06a6 does.

@matthemsteger
Copy link
Author

@dgutov I did try that, unless I messed it up, it did not work. That being said, I had not realized rjsx-mode had rjsx-minor-mode, so using that solves the js2-minor-mode issues with jsx it seems. So using js-mode, js2-minor-mode, rjsx-minor-mode and tree-sitter-mode seems to get pretty close to functionality and syntax highlighting, without a couple of the nice features of rjsx-mode for JSX in particular.

@guillaumebrunerie
Copy link

I tried to remove the indent-tabs-mode binding (and the .elc file as well, to make sure it runs the modified code), and it kind of works, except that in the JSX parts it mixes tabs and spaces in order to get the 2 spaces indents (tabs are 4 spaces for me).

@dgutov
Copy link

dgutov commented Jul 5, 2021

except that in the JSX parts it mixes tabs and spaces in order to get the 2 spaces indents

That's what indentation with tabs in Emacs does. To avoid it, make sure all offsets are multiples of the tab width.

Did you expect some other behavior?

@guillaumebrunerie
Copy link

I guess it is due to #106 then.
But I did not expect some other behavior, I just mentioned it because ESLint complained a lot about "Mixed spaces and tabs", so I didn't want to simply say "it works". But you’re right that it is just the normal behavior of indentation with tabs in Emacs.

@genovese
Copy link

Just as a followup, I usually use spaces but have been working on a project where tabs are used and so needed this. I tracked down the binding change that @felipeochoa suggested and it has been working for me so far. Just saw this issue and wanted to give feedback.

Thanks for rjsx-mode. It's been very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants