-
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
Allow all types as a key in $PropertyType
.
#2952
Conversation
Fixed build error in |
@samwgoldman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Oh, interesting. It looks like the rules for Without that, code like this fails unexpectedly:
If you are interested in something else to work on, we'd definitely need to resolve this issue before landing this change. If you're busy, I would be happy to take it on. |
Thanks for reviewing! I will finish it. Need this for relay typings.
… On Dec 14, 2016, at 3:31 AM, Sam Goldman ***@***.***> wrote:
Oh, interesting. It looks like the rules for InstanceT _, (GetElemT _ | SetElemT _) are a bit dumb and don't attempt to convert statically-known strings to named property lookups the way the corresponding rules for ObjT do.
Without that, code like this fails unexpectedly:
class C { p: number }
(0: $PropertyType<C, 'p'>);
If you are interested in something else to work on, we'd definitely need to resolve this issue before landing this change. If you're busy, I would be happy to take it on.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Why do not you integrate this PR? |
I also need this feature as well! |
@pvolok @samwgoldman |
Was this fixed in 968210c? |
@bradencanderson Or, for an example that may be more relevant to many followers of this issue:
|
|
@pvolok why it is closed? My guess you are not able to work on this anymore - can you comment if this is the case? |
@Lapanoid Looks like |
@pvolok Ahh I see, I thought it is released already, but it is only in master currently |
@Lapanoid Yeah, only landed in master on Sunday :-P |
No description provided.