search

windwp / nvim-ts-autotag

Use treesitter to auto close and auto rename html tag
MIT License
1.63k stars 87 forks source link

fix: tsx node renaming #201

Closed inferst closed 2 months ago

inferst commented 2 months ago

Hello!

I've faced with issue when renaming tags works differently for tsx and jsx files. Sometimes renaming doesn't work for tsx nodes, but it's fine with jsx.

I think it's related to #190

The problem is vim.treesitter.get_node_text return empty lines and lines with spaces for tsx nodes so validation failed.

I'm not sure that this is the right way to fix it. But I'm not familiar with treesitter. Probably it should be fixed there or in parser.

How to check it:

Just try to rename any node in tsx file https://github.com/inferst/vite-react-ts/blob/master/src/App.tsx

Without fix:

https://github.com/user-attachments/assets/1eb6af26-da22-40a6-8a8e-840bce19f738

With fix:

https://github.com/user-attachments/assets/564a1bda-cfe6-4e3b-859f-062738be57ef

PriceHiller commented 2 months ago

Hey there, thanks for the PR! Apologies for missing this yesterday 😅.

I'll be looking at this more in depth later today in the afternoon.

Based on a cursory glance though (from my phone), would it be possible to reparse the tree if the initial text for the node comes up empty? If not, no problem, I'm quite literally throwing an idea at the wall and hoping it sticks -- not actually at a computer to test that idea.

inferst commented 2 months ago

@PriceHiller Hi!

I am not sure that I get it correctly. Do you mean to reparse the tree instead of just fix validation function? Probably in this case we mustn't use vim.treesitter.get_node_text.

PriceHiller commented 2 months ago

I am not sure that I get it correctly. Do you mean to reparse the tree instead of just fix validation function? Probably in this case we mustn't use vim.treesitter.get_node_text.

Don't worry about it, my comment was in error. If you can get those tests passing that would be great!

As it stands, I think all rename behavior for tsx/jsx is broken on main, so this is a major improvement over the main branch. To be more practical, if you can't get that test passing, I'll merge this at your request and go from there with the future intent being to fully resolve these issues.

inferst commented 2 months ago

Don't worry about it, my comment was in error. If you can get those tests passing that would be great!

As it stands, I think all rename behavior for tsx/jsx is broken on main, so this is a major improvement over the main branch. To be more practical, if you can't get that test passing, I'll merge this at your request and go from there with the future intent being to fully resolve these issues.

The problem of other tests is in rescript files. I tried to investigate it a bit and even couldn't install the parser, so may be the problem is here :)

I suggest to merge what we have now.

Also I moved this fix from validation function to utils.get_node_text. I think this is more correct. Typescript tests passed.

PriceHiller commented 2 months ago

I suggest to merge what we have now.

Sounds good to me 🙂.

Seems we still have a test failure going on, which is irrititating, but this does resolve most test failures so we'll roll with it.

Thanks for the work here!