Incorrect syntax highlighting with yats.vim about vim-jsx-pretty HOT 18 CLOSED

Hi, @sheerun @sQVe @davidroeca @tsuyoshicho This issue should have been fixed in #99. Update this plugin and take a look, Thanks.

from vim-jsx-pretty.

Created a simple repo that reproduces this issue both for flow and typescript https://github.com/davidroeca/vim-jsx-pretty-issue-86-repro

from vim-jsx-pretty.

Then it's at least good clue :)

from vim-jsx-pretty.

@sQVe If that's issue with yats.vim maybe you'll open an issue there?

from vim-jsx-pretty.

@davidroeca Thanks for your investigation. This regex is indeed complex because we have to use it to detect the jsx tag start boundaries and the boundaries vary greatly.

Currently, it cannot be simplified because this plugin has no assumption on which plugin (vim-javascript or typescript-vim) is used.

I will try to fix this issue in a few days.

from vim-jsx-pretty.

Thanks for your feedback, will take a look in my spare time.

from vim-jsx-pretty.

@sheerun I can reproduce this issue without vim-jsx-pretty. So this should be an issue of yats.vim.

from vim-jsx-pretty.

@sheerun Closing this issue since it's not an issue of this plugin.

from vim-jsx-pretty.

@yuezk, @sheerun: The indenting is an issue with yats.vim. The issue (sheerun/vim-polyglot#428) referred to highlighting. As soon as I add vim-jsx-pretty the following breaks typescript (both yats.vim and typescript-vim) highlighting for all code below it:

const head = <T>(arr: T[]): T => arr[0]

I suggest reopening and renaming this issue or alternatively creating a new issue that tracks the problematic highlighting.

from vim-jsx-pretty.

@sQVe OK, I see. I'll reopen this issue.

from vim-jsx-pretty.

I think the highlighting of generic functions is an issue here around how a JSX group is defined. I've encountered the same issue in https://github.com/leafgarland/typescript-vim and also in https://github.com/pangloss/vim-javascript with flow.

from vim-jsx-pretty.

My best guess at a fix would be an edit in the following lines:

But it's tough for me to be sure; the regex is giving me a bit of a headache 😅

from vim-jsx-pretty.

@davidroeca @sQVe @sheerun

I think this issue is hard to fix and I'm trying to explain it.

Vim syntax highlighting has its limitation because it uses the regex to match the syntax and it's a static match, for most of the cases, this mechanism works well since most of the language syntax is unambiguous.

But the test case in this issue is ambiguous.

const head = <T>(arr: T[]): T => arr[0]

<T> can be the start element of a jsx element, it can also be the generic type definition, it's based on the syntax after it. But when regex syntax matches, it can not parse the syntax after it, so it treats the <T> as the start element of the jsx element.

A possible solution is that we make an assumption that if <T> is followed by a (, we treat it as the generic type. But this may break the highlight of const head = <T>(...)</T>, which is a valid jsx element.

The workaround is to use the function expression:

const head = function <T>(arr: T[]): T { return arr[0]; }

or use the trailing comma after the type

const head = <T,>(arr: T[]): T => arr[0]

Reference: microsoft/TypeScript#15713

from vim-jsx-pretty.

@yuezk I understand. Thank you for diving deep into this issue 🙏

I mostly write JavaScript / TypeScript in a functional manner so the likelyhood of me writing functions like const head = <T>(arr: T[]): T => arr[0] is quite a bit larger than using a <T>(...)</T> JSX block. The JSX counterpart is also easier to repair with newlines etc.

It would be interesting to know how common the <T>(...)</T> case really is. I'm unsure on what that specific use case solves.

I vote to fixing the generic type case even though it might introduce a very specific issue with JSX.

from vim-jsx-pretty.

@sQVe You can update this plugin and use the workaround by adding a trailing comma after T.

const head = <T,>(arr: T[]): T => arr[0]

from vim-jsx-pretty.

I also agree that const head = <T>(arr: T[]): T => arr[0] is probably more popular than <T>(...)</T>, so probably checking if <T> is followed by a ( is fair indicator (or checking if </T> is present in the same line).

Also good indicator is that it's capital case e.g. in my codebase there's <span>({photos.length})</span> but it cannot be generic type because it's lower case.

from vim-jsx-pretty.

Also good indicator is that it's capital case e.g. in my codebase there's <span>({photos.length})</span> but it cannot be generic type because it's lower case.

Using a capital letter for a generic type is considered good practice but it is not required. The following snippet is valid TypeScript:

const head = <t>(arr: t[]): t => arr[0]
// or even
const head = <span>(arr: span[]): span => arr[0]

from vim-jsx-pretty.

@yuezk thanks for taking the time here!

I’m good with either approach as long as what correctly highlights is compatible with what linters and prettier expect. I know prettier removes unnecessary parens. I’d assume most standard linter configs would complain about lower-case type variables.

from vim-jsx-pretty.