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

fix: tsx node renaming #201

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Conversation

inferst
Copy link
Contributor

@inferst inferst commented Jul 13, 2024

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:

wofix.mp4

With fix:

wfix.mp4

@PriceHiller
Copy link
Collaborator

PriceHiller commented Jul 14, 2024

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
Copy link
Contributor Author

inferst commented Jul 15, 2024

@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
Copy link
Collaborator

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
Copy link
Contributor Author

inferst commented Jul 16, 2024

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
Copy link
Collaborator

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!

@PriceHiller PriceHiller merged commit 1624866 into windwp:main Jul 16, 2024
0 of 2 checks passed
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