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

Default value of an object pattern property should not change type if it's assignable #189

Closed
kdy1 opened this issue Nov 4, 2022 · 4 comments · Fixed by #221
Closed
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@kdy1
Copy link
Member

kdy1 commented Nov 4, 2022

export interface BarProps {
barProp?: string;
}
export interface FooProps {
fooProps?: BarProps & object;
}
declare const foo: FooProps;
const { fooProps = {} } = foo;
fooProps.barProp;

This should success.

image

access_property of ((BarProps#1 & object) | {}) is expected to fail, because {} does not have the property.
But the type of fooProps should be (BarProps#1 & object) (without union)

I think the logic at

if prop.value.is_some() {
prop_ty = prop_ty.map(|ty| {
// Optional
ty.map(|ty| {
Type::new_union(
span,
vec![
ty,
Type::Keyword(KeywordType {
span,
kind: TsKeywordTypeKind::TsUndefinedKeyword,
metadata: Default::default(),
}),
],
)
})
});
}
is problematic, and I think we should adjust it to ignore default value if the type of the default value is assignable to the original type.

@kdy1 kdy1 added bug Something isn't working good first issue Good for newcomers labels Nov 4, 2022
@kdy1 kdy1 added this to the 1. Correctness milestone Nov 4, 2022
@kharvd
Copy link
Contributor

kharvd commented Nov 5, 2022

Hi @kdy1, I'd like to give it a try!

@kdy1
Copy link
Member Author

kdy1 commented Nov 5, 2022

Thanks!

@kharvd
Copy link
Contributor

kharvd commented Nov 6, 2022

Is the object part relevant here? Looks like tsc infers BarProps even when fooProps has type BarProps, without the object intersection.

I would guess we should infer BarProps | DefaultValue if DefaultValue is not assignable to BarProps & object, and just BarProps otherwise. {} is assignable to BarProps & object (or BarProps fwiw), so we should infer BarProps & object

@kdy1
Copy link
Member Author

kdy1 commented Nov 6, 2022

I think you are right, nice catch! Thank you.

I tried incompatible default value and it matches with your observation

@kdy1 kdy1 changed the title Default value of an object pattern property should not change type Default value of an object pattern property should not change type if it's assignable Nov 6, 2022
@kdy1 kdy1 closed this as completed in #221 Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
2 participants