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: hoist types related to $props rune if possible #2571

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Conversation

dummdidumm
Copy link
Member

This allows TypeScript to resolve the type more easily, especialy when in dts mode. The advantage is that now the type would be preserved as written, whereas without it the type would be inlined/infered, i.e. the interface that declares the props would not be kept

@jasonlyu123 does the approach make sense to you? Anything I might have missed?

This allows TypeScript to resolve the type more easily, especialy when in dts mode. The advantage is that now the type would be preserved as written, whereas without it the type would be inlined/infered, i.e. the interface that declares the props would not be kept
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.

These two also cause some errors.

    interface Props {
        hi2: C;
    } 
 
    const a = 1
    type C = typeof a | '2' | '3';
    interface Props {
        hi2: C;
    } 
 
    const a = 1
    type C = typeof a | '2' | '3';

@dummdidumm
Copy link
Member Author

These two also cause some errors.

I only see one code snippet two times 😅

Fixed the bugs you pointed out, thanks!

@dummdidumm dummdidumm merged commit bf2e459 into master Nov 15, 2024
3 checks passed
@dummdidumm dummdidumm deleted the hoist-types branch November 15, 2024 12:54
@jasonlyu123
Copy link
Member

Oops. Copied the wrong thing and I don't even remember what is the other case either 😆. Probably just a variant of the same issue I guess.

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