-
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
Regression in invalid-computed-prop detection #9141
Comments
This is due to the fact that things like
are never safe, and just returns any before. In the latest Flow version, we choose to start loudly error. |
I tried your initial example, locally over two files, and was not able to reproduce the error. |
I struggled a bit but I eventually understood what I did wrong. |
Well, there's still something bothering me. enum MyEnum {
ValFoo = 'foo',
ValBar = 'bar'
}
declare const obj: {foo: string, bar: string};
declare const key: string;
obj[MyEnum.ValFoo as string] Why doesn't it work, since |
In fact, I still don't understand the difference between (1): type Dict<T> = {| [string]: T |};
export type BoolDict = Dict<bool>; and (2): export type BoolDict = {| [string]: bool |}; The following code does not work with (1) but is OK with (2): const bd: BoolDict = { foo: true };
const k = 'foo';
const {[k]: v} = bd; |
The enum example is an issue with enum feature. The right solution is to support enum key rather than continue to allow this unsafe access.
I am still unable to reproduce this? Can you provide a try-flow link? |
Unfortunately, I can't. |
Flow version: 0.235.1
Expected behavior
My code looks like this:
Then, in another file, I have:
Until flow-bin v0.234.0, no error was raised.
Actual behavior
Since I upgraded to flow-bin v0.235.1, the line
const {[k]: v} = bd;
raises the following error:I tried to reproduce on flow.org/try but to no avail.
But I tried the following code, which does not raise any error:
The other part remains unchanged:
So my guess is it has something to do with the generic type, or the export, or a combination of both.
Thanks for your help.
The text was updated successfully, but these errors were encountered: