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

Top-level jsx elements cannot be uncommented #29

Open
desi-belokonska opened this issue Nov 3, 2021 · 8 comments · May be fixed by #56
Open

Top-level jsx elements cannot be uncommented #29

desi-belokonska opened this issue Nov 3, 2021 · 8 comments · May be fixed by #56
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@desi-belokonska
Copy link

comment-issue.mov

Problem

Commenting inner jsx elements works fine, but when doing it on the top-level element it only works the first time - every subsequent toggling just uses the default // comment.

Possible source

Did some digging - the function

function M.get_node_at_cursor_start_of_line(only_languages, winnr)

returns a <node jsx_opening_element> the first time and it correctly comments it out with {/* %s */}. When trying to uncomment, however, it returns a <node object>, possibly because it recognizes the { as the start of an object.
Hope this helps!

@JoosepAlviste
Copy link
Owner

Hey @desi-belokonska! I've also run into the same issue (and there's an issue for this already as well: #11).

You're correct in the source of the issue. But I'm wondering what the correct behaviour in this case would even be to be honest. It feels like // would be the more correct commentstring because this is valid JSX:

(
  // <div>
    <h1>Hello</h1>
  // </div>
)

But this is invalid:

(
  {/* <div> */}
    <h1>Hello</h1>
  {/* </div> */}
)

So, it seems like the perfect solution would be to check if jsx_element is wrapped by parenthesized_expression. In that case, we should use the // commentstring instead.

I had an idea for a way to configure parent element checks similarly to this:

  tsx = {
    jsx_element = {
      __default = '{/* %s */}',
      __parent = {
        parenthesized_expression = '// %s',
      },
    }
  },

This would also probably fix #22. However, the parent queries would even be worth the effort to implement.

Anyways, I don't think I'll implement this in the near future, but it would be a nice addition and I would gladly accept a contribution 😃

@vishalbalaji
Copy link

vishalbalaji commented Aug 30, 2022

Hello, I've facing the same issue in JSX code and there seems to be no activity regarding this. Just want to make sure whether there is a solution to this issue and that I am not missing something.

@JoosepAlviste
Copy link
Owner

Hey @vishalbalaji! This would be a pretty big change and I haven't really had a chance to look into it. Since it's such a small edge case, then I'm not even sure if there is any point in spending too much time on it. I would gladly accept a PR though 😄

Personally, I don't think being able to comment out the first line of JSX is all that useful (don't think I've ever wanted to do this), but maybe I'm missing something?

@vishalbalaji
Copy link

Hey @JoosepAlviste, I have mainly noticed this being an issue when commenting out entire components. All the code even within the component becomes uncommentable as seen in the GIF below. I'm not entirely sure, but I believe this is because the parent tag is then identified as ERROR by treesitter as the commented top-level tag is not valid JSX.

Peek 2022-08-30 20-16

2022-08-30_20:20:22_scrot

@AlexanderHott
Copy link

Hey @JoosepAlviste, I have mainly noticed this being an issue when commenting out entire components. All the code even within the component becomes uncommentable as seen in the GIF below. I'm not entirely sure, but I believe this is because the parent tag is then identified as ERROR by treesitter as the commented top-level tag is not valid JSX.

Are you saying that its a bug in treesitter?

@JoosepAlviste
Copy link
Owner

I don't think it's really a problem in Treesitter or any parsers, because this is not valid jsx syntax and probably should result in a broken syntax tree:

(
  {/* <div> */}
   <h1>Hello</h1>
  </div>
)

Ideally, this plugin should probably be smart enough to be able to comment from the first line of JSX like so:

(
  // <div>
    <h1>Hello</h1>
  </div>
)

Which should mean that the first line can be un-commented based on the standard rules we already have.

Currently, there's no easy way to configure this plugin to handle the above case, but something like what I mentioned above (#29 (comment)) should make this possible. Implementing that would probably take quite a bit of effort though.

It seems like this is a rather important issue for folks though, so we could hard-code a workaround in this plugin. Something like "if language is jsx and we're on the first line of a JSX statement, use a standard JS comment". It shouldn't be too tricky to get this to work, probably just requires some fiddling.

I don't know how much time I would have to work on this though, so I would gladly accept a PR with a similar hacky solution 😄

@AlexanderHott
Copy link

I'd try, but I barely know lua. Is there a good place to learn neovim plugin development? @JoosepAlviste

@Fedex159
Copy link

Hi, I have the same problem and I want to help in some way. I made a simple keymap to solve this specific cases.

vim.keymap.set("v", "<leader>gc", ":s/{\\/\\* // | '<,'>s/ \\*\\/}//<cr> | :noh<cr>", { silent = true })

Maybe, because this is a tricky case, we can add some extra keymaps or some config that let config a disabled keymap to execute some similar code as the keymap I put above, I don't know.

video.mp4

JoosepAlviste added a commit that referenced this issue Dec 31, 2022
The first and last lines of JSX should be commented with `// %s`, rather
than `{/* %s */}`. However, the current way of configuring the plugin
does not allow us to check if the current node is a `jsx_element` with a
parent of `parenthesized_expression`. We could allow checking the parent
node in the configuration, but that would be a bit tricky and a lot of
work.

Since this is (hopefully) a 1-time exception, then it seems okay to add
an explicit check for this. If we have similar cases in the future
though, then we should think of making this configurable somehow.

Fixes #29
@JoosepAlviste JoosepAlviste linked a pull request Dec 31, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants