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

Nested ExpressionStatements Issue #144 #181

Merged
merged 4 commits into from
Aug 11, 2022
Merged

Conversation

Calidus
Copy link
Contributor

@Calidus Calidus commented Aug 8, 2022

As discussed in #144 ts-migrate will duplicate code when the add-conversion plug in is ran. This fix addresses the situation when there are nested expression statements that need to have as any applied. Previously nested expression statements would generate multiple overlapping updates, which results in duplicate lines of code one with the conversions changes and one without.

There a number of things I don't know which I would appreciate feedback/help with:

  • Are there other combinations of syntax that generate similar issues?
  • How can I test these changes on an existing code base? I don't know much about node.
  • All the original tests still pass, is there additional testing that should be doing?

Calidus added 3 commits August 8, 2022 14:18
- Prevents code duplication when nested nested statement
expressions need are run though the `add-conversions` pluggin.
- Previously add-conversions would generate two over lapping updates
which `apply()` can not handle correctly.
- This fix handles nesting other stantments inside of a expression statement
- Added additional test
@Calidus
Copy link
Contributor Author

Calidus commented Aug 9, 2022

If this approach seems correct, we can probably come up with a way to replace the recursive ancestorIsExpressionStatement function by modifying visit.

- Use a map to keep track of which nodes are expression statments
- Check the map when deciding if we should replace a node
@Rudeg
Copy link
Contributor

Rudeg commented Aug 11, 2022

Are there other combinations of syntax that generate similar issues?
All the original tests still pass, is there additional testing that should be doing?

It should be fine.

How can I test these changes on an existing code base? I don't know much about node.
You can try to run a ts-migrate version from the master on the existing codebase.

I think it should be good to merge and you can test it with next release.

Thank you for the contribution!

@Rudeg Rudeg merged commit d0c3926 into airbnb:master Aug 11, 2022
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