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

Typescript tsx improvements #15895

Closed
wants to merge 8 commits into from

Conversation

mkcode
Copy link
Contributor

@mkcode mkcode commented Jan 18, 2023

A few improvements for typescript-tsx mode.

  • Fixes mismatched indention for tsx files when indenting using LSP
  • Uses tree-sitter for highlighting tsx files, if it's available.
  • Workaround to fix a conflict with web-mode fontification and tree-sitter

layers/+lang/typescript/funcs.el Outdated Show resolved Hide resolved
layers/+tools/lsp/packages.el Outdated Show resolved Hide resolved
@mkcode
Copy link
Contributor Author

mkcode commented Jan 18, 2023

@lebensterben - Changes made. Thanks for the quick feedback.

Copy link
Contributor

@lebensterben lebensterben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mkcode
Copy link
Contributor Author

mkcode commented Jan 19, 2023

Please wait on merging this. I am experiencing some strange effects of the tree-sitter tsx highlighting.

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

Ok. I threw the new tree-sitter related behind an opt-in feature flag / custom variable, so I now consider this safe to merge. Users can enable/disable tree-sitter tsx highlighting as they wish.

@lebensterben
Copy link
Contributor

additional note:

@lebensterben
Copy link
Contributor

the question is, should we add an extra layer config variable to toggle tree-sitter highlight? or can we teach user to modify the two aforementioned variables?

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

the question is, should we add an extra layer config variable to toggle tree-sitter highlight? or can we teach user to modify the two aforementioned variables?

My feeling is that this using tree-sitter for tsx should be opt-in rather than opt-out. For 2 reasons:

  1. I am not yet confident of the reliability of tree-sitter tsx mode highlighting.
  2. It involves a custom require and workarounds to get it working, due to the conflicts with web-mode. See the following:
    https://github.com/syl20bnr/spacemacs/pull/15895/files#diff-b129d16c228863d84fd5cc295c6f1de9d0170d359e7231f271ed6a2ac746529fR107-R109

In spacemacs, we inherit our tree-sitter-major-mode-language-alist value from the tree-sitter-langs package. But because there is no canonical emacs tsx mode package, we can't ask them to upstream any change for tsx.
https://github.com/emacs-tree-sitter/tree-sitter-langs/blob/6e54f2d04612c2e1b439bb0ffc9d25cce7c72029/tree-sitter-langs.el#L106

And after thinking about this some more, the potential behavior issues of the tree-sitter highlighting and the workarounds are only needed because we are deriving our tsx mode from web-mode, when deriving it from typescript mode may yield much better results. My initial test is working well enough, and perhaps adding back some web mode nice-to-haves like emmet and such to a typescript-mode derived tsx-mode would solve all of these issues at once.

@lebensterben
Copy link
Contributor

See https://github.com/emacs-typescript/typescript.el/blob/master/README.md#good-news-though

It seems that we may not need typescript-tsx-mode for the upcoming Emacs release. Are you willing to test that? (we don't have to adapt it in this PR though unless you want)

@lebensterben
Copy link
Contributor

Actually

https://github.com/emacs-typescript/typescript.el/blob/4fcb4594819caf472ae42ea068a1c7795cf07f46/typescript-mode.el#L3121-L3122

this line tells Emacs to open .tsx files in typescript-mode. So it indeed seems that we may not need to define typescript-tsx-mode even before Emacs 29 is released.

I've never developed in TSX so I don't know what are the goodies from web-mode that we will want to keep for TSX.

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

That is great news. Thanks for the research.

But then, I read this in the official tree-sitter typescript definition repo, which leads me to think that we really should be using different modes, or at least this is important if we want to use tree-sitter:

Because TSX and TypeScript are actually two different dialects, this module defines two grammars.

See:
https://github.com/tree-sitter/tree-sitter-typescript/tree/3e897ea5925f037cfae2e551f8e6b12eec2a201a#tree-sitter-typescript

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

Ok! After researching this a bunch more, I wan't to proceed with the following plan:

We have excellent new modes that are included with emacs 29. typescript-ts-mode for typescript and tsx-ts-mode for tsx. See the emacs-29 branch, there is nothing typescript related in the emacs-28 branch: https://github.com/emacs-mirror/emacs/blob/emacs-29/lisp/progmodes/typescript-ts-mode.el#L379

I want to update this mode to simply use these packages if they are available. I am using emacs-29 anyway, so this is certainly the best solution for me.

Questions on best implementation:

  1. Should we do a emacs-version check here? or is there a better way to test if these modes are available. I would like to not use the typescript-mode (and web-mode) package if we don't need it.
  2. If we potentially have two differently named modes, IE typescript-tsx-mode if using emacs-28 and tsx-ts-mode if using emacs-29, how should we account for the potentially different mode names when say, adding a hook? Is there an established best pattern for handling cases like this? Do I just hold the mode name in a variable? Or perhaps this should just be an config option for which to use, and setting tsx-ts-mode on emacs-28 will simply raise an error saying this only available for emacs-29.

Thank again for your time with this.

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

I think using an option like this would be best:

(defvar typescript-use-treesitter-modes nil
  "Use the emacs-29 provided typescript-ts-mode and tsx-ts-mode rather then typescript-mode. Setting this option to non-nil requires emacs-29.")

Let me know what you think.

@lebensterben
Copy link
Contributor

since the unofficial typescript-mode has hit a halt: https://github.com/emacs-typescript/typescript.el#a-short-note-on-development-halt

we eventually need to step away from it. but that cannot be done before spacemacs-emacs-min-version becomes 29.1.

For the time being, we can use version< and emacs-version for version checks.

(if (version< emacs-version "29.0")), we still has to use all the packages we are currently using, with the proposed changes you made.

Otherwise, we simply use the major-modes of TS or TSX provided by Emacs. And they will have native support for tree-sitter highlighting. (see https://github.com/emacs-mirror/emacs/blob/d0d34514097c03d787012478d5217449481cfc04/lisp/progmodes/typescript-ts-mode.el#L360-L361 and https://github.com/emacs-mirror/emacs/blob/d0d34514097c03d787012478d5217449481cfc04/lisp/progmodes/typescript-ts-mode.el#L384-L385).

As for discrepancies in symbol names, we can also use the same version checks. This is the most straightforward way.

@mkcode
Copy link
Contributor Author

mkcode commented Jan 20, 2023

Ok. Thanks. I'll proceed with using just the version check and no defvar to specify.

@mkcode
Copy link
Contributor Author

mkcode commented Feb 15, 2023

@lebensterben - After looking into this issue a bunch more, I'm concluding that this work is just too early to get this into spacemacs, and should be added whenever spacemacs approaches full emacs 29 support.

Spacemacs should support automatic bootstrapping of tree-sitter language grammars, if it is going to use the new emacs29 language modes that leverage tree-sitter. This project and issue is looking to support this eventually, and I would think should be used in spacemacs eventually to DL tree-sitter language grammars: emacs-tree-sitter/tree-sitter-langs#144.

In the meantime, I opened up #15920, with the non tree-sitter related improvements in this PR.

@mkcode mkcode closed this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants