Skip to content

Conversation

@dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Feb 26, 2021

This adds an IfScope to svelte2tsx which keeps track of the if scope including nested ifs. That scope is used to prepend the resulting condition check to all inner lambda functions that svelte2tsx needs to create for slot, each, await. This way, TypeScript's control flow can determine the type of checked variables and no longer loses that info due to the lambda functions.
#619

TODO:

  • handle shadowed variables
  • remove hidden checks from getDefinitions/findReferences/diagnostics
  • fix rename

This adds an IfScope to svelte2tsx which keeps track of the if scope including nested ifs. That scope is used to prepend the resulting condition check to all inner lambda functions that svelte2tsx needs to create for slot, each, await. This way, TypeScript's control flow can determine the type of checked variables and no longer loses that info due to the lambda functions.
sveltejs#619
@dummdidumm dummdidumm marked this pull request as draft February 26, 2021 14:15
@dummdidumm dummdidumm marked this pull request as ready for review March 4, 2021 12:12
@dummdidumm
Copy link
Member Author

@jasonlyu123 could you review this? The logic is a little complicated and I changed code in quite a few places (although most of the new code is tests) and I want to make sure I'm not doing something stupid here or if the approach I took here is not good in your opinion.

High level summary: The if-conditions are repeated and prepended to the lambda functions to reinstantiate control flow. I introduced comments around them to be able to filter those repeats out when returning things like diagnostics,rename etc. That comment could possibly used in other places, too, later on.

Copy link
Member

@jasonlyu123 jasonlyu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great. Didn't found many problems. Just some minor one

Copy link
Member

@jasonlyu123 jasonlyu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

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