-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use SingletonStrT, SingletonNumT, SingletonBoolT in case of const literal declarations #7607
base: main
Are you sure you want to change the base?
Conversation
@mrkev probably |
Let's check this out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I'm seeing issues similar to this:
Also tagging @nmote so this PR get some visibility from someone in the team. |
@mrkev I'm not sure how this is related, because it only uses affects const primitive literals, not object literals properties, I'll check |
Maybe you mean something like this const bar = 10;
const foo = {
bar,
baz(x: number) {
foo.bar = x; // Cannot assign `x` to `foo.bar` because number [1] is incompatible with number
},
} |
Yeah, totally bug, I should replace |
Now this errors correctly
|
@mrkev do you think this shouldn't error at all and property type should be widened to number? |
It's interesting that in TypeScript if type is inferred then it's widened, but if it's specified by user it isn't const str = 'string';
const num = 200;
const bool = true;
const str1: 'string' = 'string';
const num1: 200 = 200;
const bool1: true = true;
const foo = {
str,
num,
bool,
str1,
num1,
bool1,
method() {
foo.str = 'any string' // no error
foo.num = 300 // no error
foo.bool = false // no error
foo.str1 = 'any string' // error
foo.num1 = 300 // error
foo.bool1 = false // error
}
} |
I think it depends if this code is bug or not |
I'm now not sure what the cases for this are, refinements and switch seems to work fine with just regular const |
I think what typescript does falls in the right direction, and would personally err on the side of widening, though this is a good question to bring up on the Discord channel for discussion IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
So this is stalled because we didn't found out how to widen literals to |
@mrkev what you think about implementing const assertions instead? EDIT: nvm, same problems with inference The only working thing is fixing |
Related issue #7961 |
declare var foo: {+a: 'str'}
declare var foo_a: 'str';
const bar = {
a: foo.a,
b: foo_a,
method() {
bar.b = 'bar' // error
bar.a = 'bar' // no error
}
} Kinda strange, how can we get |
2fea3f5
to
01ef2d2
Compare
@mrkev I solved this problem, should I add more tests? |
This actually makes it inconsistent with but it is separate issue |
Revived #4175
Fixes #2639