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(#199): Auto-tag react fragments #202

Closed roycrippen4 closed 2 months ago

roycrippen4 commented 2 months ago

Auto-closes react fragment tags in js, jsx, and tsx files.

PriceHiller commented 2 months ago

Assigning myself as a reminder to look at this in a bit when I'm available.

roycrippen4 commented 2 months ago

@PriceHiller No problem. I'm writing a few more tests to ensure it's working correctly.

I know there's a possible issue in .js files if there isn't any jsx specific syntax to match against. If you just open a new .js file and the first jsx syntax is a react fragment it won't work. I don't think this is necessarily a bad thing as it'll be almost impossible to handle such a case.

This shouldn't be an issue in .jsx or .tsx files since the file-type is checked.

roycrippen4 commented 2 months ago

I found a couple issues when running the tests. I'm working on a fix now.

PriceHiller commented 2 months ago

If the tests fail due to incompatibility between this and a less important feature (like automatically adding a closing tag for an opening tag without a closing partner) and it proves to be too difficult to resolve the compatilibty between them just list out the compatability issues and we can decide if it's worth losing something for this.

Fragment closing seems to be quite popular in terms of requested features so I'm ok with losing lesser known/used features for it.

If that turns out to be the case though, I'll be marking this as the start of autotag 1.0.0 and yanking some things that have been marked as deprecated for a while.

roycrippen4 commented 2 months ago

It should all be compatible. I just made an error with boolean logic and need one extra check function and (hopefully) everything should work. There are a couple of tests failing on the main branch. I'm making my measure of success as getting the same tests that fail on main to match the PR's tests.

roycrippen4 commented 2 months ago

I'm pretty sure those are the same tests that are already failing on the main branch. Are those tests related to #190 and #196?

PriceHiller commented 2 months ago

I'm pretty sure those are the same tests that are already failing on the main branch. Are those tests related to #190 and #196?

Yeah, those tests appear to be failing due to an update upstream in nvim-treesitter for the js/ts parsers.

Probably not your fault based on what I'm seeing locally. Entirely separate issue (though it is a problem).

roycrippen4 commented 2 months ago

We should make an issue to get them fixed so at least it's documented.

PriceHiller commented 2 months ago

We should make an issue to get them fixed so at least it's documented.

Already done 😉 (#205)

PriceHiller commented 2 months ago

Ping me when you're good to go over here (or convert this PR to a draft and then promote it when you're ready) and I'll take a look

roycrippen4 commented 2 months ago

It's good to go

roycrippen4 commented 2 months ago

I mentioned it in #199, but this PR doesn't fix #185.

PriceHiller commented 2 months ago

Also, for clarification's sake, do I need to close #185 once this is merged?

roycrippen4 commented 2 months ago

No, #185 will get it's own PR. I probably won't get it done today, but in the near future.

PriceHiller commented 2 months ago

No, #185 will get it's own PR. I probably won't get it done today, but in the near future.

Gotcha, no rush on it. You're doing god's work right now. I'll leave that issue alone then.

Just hit it with a make format and I think we're good to go.

PriceHiller commented 2 months ago

Awesome, thanks again @roycrippen4 -- great work!

roycrippen4 commented 2 months ago

Happy to help!

abilkhan024 commented 1 month ago

Is there a way to opt out from this feature, or potentially change current behavior. Right now it causes problems when i want to pass type arguments by adding </>. For example: typing: useState<> results: useState<></> I didn't find work around for that, pretty sure that was not intentional behaviour.