-
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
Type annotations inside destructuring #235
Comments
We've held off on adding inline type annotation for destructuring since there isn't an obvious place in the grammar for object destructuring ( Also I don't think TypeScript supports these kinds of annotations, so it wasn't something we urgently needed to support. You're saying the 6to5 acorn fork has support? |
@gabelevi Yeah, I was rewriting some of the flow parsing in |
Hmm, interesting how TypeScript / Flow are going to resolve this. Ideally, we could just do it during scope analyzing so if "property value" of object pattern is linked to type, then use it as type annotation and if not, use as destructuring target. The only downside will be for cases like: class A {}
var {a: A} = obj; when you really want to override locally defined |
@RReverser You can just do: class A {}
var { a }: { a: A } = obj; |
@sebmck That doesn't look good enough - variables and their types are split into different sides of operator. It works for regular object literals, but doesn't look so nice for bindings. |
I've been thinking about this for awhile, and I think the // Works well for object types
var { number x; string y } = { x: 123, y: "hello" };
// Works well for object patterns
function foo({number a, string b}) {}
// Not so sure if object literals need this
var z = {
?string m: null,
?number n: null
};
// because soon Basil's new expression annotation syntax will land
// which will help object literals
var z2 = {
m: (null: ?string),
n: (null: ?number)
} The downsides include that this paradigm doesn't match the other type annotations and that there are certain reserved keywords in the modifier position ( |
@gabelevi, maybe I am being dense here, but why is |
Oh, nevermind. I forgot about rebinding destructuring. |
TypeScript uses var { x as number, y as string } = { x: 123, y: "hello" };
function foo({a as number, b as string}) {}
var z = {
?m: null as string,
?n: null as number,
}; Alternatively, one could also use “Basil's new expression annotation syntax” everywhere: var { (x : number), (y : string) } = { x: 123, y: "hello" };
function foo({ (a : number), (b : string)}) {}
var z = {
m: (null : ?string),
n: (null : ?number)
} But I like |
Hmm, I actually like |
👍 to @rauschma's idea of using "as". |
👍 to @rauschma's other idea of using "expression annotation syntax", or what I naïvely call "casting". |
This would be super useful for functional React components: const Component = ({
propA,
propB,
}) =>
.... Currently, the best way is to create a type and access the props via field access (that way only type Props = {
propA: SomeType,
propB: SomeType,
}
const Component = (props: Props) =>
.... Clearly the destructuring and typing syntaxes clash :(. This makes sense to me, but is super ugly: const Component = ({...}: {
propA: SomeType,
propB: SomeType,
}) => (since empty {} destructuring seems to be going through the committee, and wouldn't be obvious anyway). const Component = ({{
propA: SomeType,
propB: SomeType,
}}) => What I'm looking for is a solution that lets us easily go from typed params to named typed params: f = (a: X, b: Y) => ...
f = ({{a: X, b: Y}}) => ... |
What about this? I like "as" operator too - it is short, simple and evident. This example is good:
|
Just bumped into this myself. I also support For some reason I thought |
This syntax is confuzy - too easy to make mistake. |
I was on a long plane ride this weekend so I figured I'd toy around with the paren-grouping syntax (i.e. On syntax: I went with what matches the most with our other annotation positions. TBH I don't find the syntax beautiful, but I don't think we're going to find something that everyone agrees on there. Additionally, after playing a bit I don't find it prohibitively inexcusable. My branch is linked below. It doesn't wire up enforcement of the destructuring annotations just yet -- but it shows that we can if we wanted to pursue this route: https://github.com/jeffmo/flow/tree/pattern_lval_annotations |
Did you get in contact with the TypeScript team? Ideally, a syntactic solution would be adopted by both Flow and TypeScript. |
@rauschma: I haven't on this, no (this was just plane-ride hackery) -- I'm happy to reach out. EDIT: I guess this isn't really casting, so maybe casting syntax consistency doesn't matter too much |
@jeffmo It's not about casting consistency, but rather would be nice to have as much syntax shared as possible. I'm pretty sure they were also exploring options for typing destructuring, so collaboration could help in both short-term and long-term perspectives. |
Also similar syntax in TS and Flow is good for IDE. |
Indeed. I will be sure we follow up with them before moving forward with anything here |
👍 |
I know that you probably can't switch the whole syntax over now, but it's too bad that we can't have this: Regular function call
Object destructuring
Array destructuring
One of the most useful places to have types is in function signatures, so it's too bad that Flow doesn't support a cohesive and readable syntax there. |
@jtremback It's not too bad at all. Flow does indeed support a cohesive and readable syntax. Destructuring aside (I'll address it below), your function (
number id,
string name
) {
// function body
} is just (by the way a function is an arrow type, you don't have a full function type there) a personal preference over function (
id: number,
name: string
): any {
// function body
} I prefer the latter, you prefer the former. It's not Destructuring actually doesn't mean that your function's argument is what you wrote it is. I'm talking about the object destructuring. Your |
Hey @avikchaudhuri thanks for taking the time to reply! I was wondering if the Flow team would consider using GitHub to discuss progress/plans/etc for Flow. It seems like a lot of it is happening through internal groups at Facebook, and I think it would be great for the Flow community if they could be a part of it. |
@gabelevi says:
@samwgoldman says:
I had the same question, and I still don't get it. What's the problem with Why would it be bad to bind x.a to string and x.b to number? (With this code, that sounds like exactly what I'd be trying to do -- make sure that x.a is a string and x.b is a number.) |
@ESRogs |
It should be noted that proposal A may be confusing if the |
What is the current working solution if I don't want a type defined externally to my function? |
@Extarys, I can't really find documentation for it, although https://flow.org/en/docs/types/objects/#toc-exact-object-types and https://flow.org/en/docs/lang/refinements/ imply it. |
As somebody late to the world of flow and this party of a thread... what's the likelihood that something along the lines of proposal B from @erykpiast's comment back in Nov 2017 will be integrated in to flow? That makes so much sense and instills DRY principles - the current implementation certainly has me wanting to just use |
@thisissami (and others finding this thread, as it's gotten pretty long): the status was summarized by @avikchaudhuri in a comment about 6 months ago:
So, the likelihood of it happening is up to you! And me, and "us" -- whoever wants to have some fun with the Flow implementation to work up a PR. |
It's been nearly three years since the solution was first mentioned and it still doesn't work. How is this not facebook's top priority with flow? I'd been using it for less than a day when I ran into this. |
Facebook’s priorities changed in November 2016, as you might remember. Sent with GitHawk |
What does it mean? Is flow no longer meant to be used for js type
declarations?
Śr., 10 paź 2018, 16:02 użytkownik Ermolay Romanov <[email protected]>
napisał:
… Facebook’s priorities changed in November 2016, as you might remember.
Sent with GitHawk <http://githawk.com>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#235 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAtQF7UfIbP3tlOtJpP-jZMRA5mRyUPcks5ujf3wgaJpZM4DXqCr>
.
|
That was a joke, it's implying that political issues are distracting Facebook from tech as a priority. It's not actually clear if the events of the last few years have made flow less important. |
Why is it so hard for everybody to understand that Facebook's main priority is for sure not their open-source software? Why can't just everybody be grateful for what they got and stop complaining all the time? It's so annoying 🙄 If you need this feature so much - fork it and contribute. That's how open-source works. |
How does this look? It incorporates current syntax like B, but it doesn't require pairs of parens. Also, the parser can't confuse it with destructuring.
|
@chaimleib Parser CAN be confused, like here:
Is this destructing or typing? |
No, in my syntax, that would be |
Try Flow gives parse errors on both
and
Both syntaxes seem to be available. But between these, I still prefer the one with colons. |
@chaimleib AFAICS types for function parameter destructuring already work just fine. if you use the appropriate Flow syntax. This thread is about variable assignment, where there is no solution. The difference is what you show is only destructuring of an existing object. The problem is creating new variables from an existing object, i.e. type-annotating those new variables. That said, looking at the original very first comment at the top, it has code that actually works just fine now. The function parameter destructuring of the array parameter can be type-annotated. Here is the variable-creating assignment from that comment. .... Aaaactually, even destructuring object assignment with new variable creation (rebinding destructuring) works now? So... what exactly is the issue now? And I'm asking for "I don't like that particular syntax" answers, it's a given that a number of people won't like whatever is chosen. |
@lll000111 Currently, we have to do things like this to get object destructuring with type annotations: const MyComponent = ({
aProp,
someOther,
zethIs,
onClick,
onSubmit
}: {
aProp: string,
someOther: number,
zethIs: Object,
onClick: (event: Event) => void,
onSubmit: (event: Event) => void,
}) => If we need to rename, add or remove a parameter, we have to modify two different lines to make sure the type annotations and the destructured fields stay in sync. And this line-doubling gets really annoying when you are destructuring lots of fields at once. We can't combine the destructuring with the annotations to get the effect we want: const MyComponent = ({
aProp: string, // actually, would rename aProp to string
someOther: number,
zethIs: Object,
onClick: (event: Event) => void, // invalid syntax
onSubmit: (event: Event) => void,
}) => So we are discussing alternate syntaxes that would let us stop repeating ourselves. My syntax proposal follows a nice summary post of other syntaxes above. |
The UX is pretty bad, it's like either flow or ES6..ES2019 😞 |
Actually we can't even rename a variable during destructuring, which is not ok at all:
Gives the following error:
I can't believe this issue has been opened more than 4 years ago and is still not addressed as it should. |
@SherloxFR You can rename a variable during destructuring. That’s the primary reason why this issue hasn’t yet been addressed: to avoid conflicting with that syntax. In your example, you need to annotate the original object that you get from const { SecretString: dbSecret }: { SecretString: string } = await secretsManager.getSecretValue() |
one could utilize cast-through-any here (unsafe!)
see also: https://stackoverflow.com/a/45068255 or, as in my case above, you could realize:
|
We banned this completely last year, since it's confusing. |
It seems like the following should be supported:
Is this a deliberate decision or was the scope of ES6 type annotations restricted until ES6 became more widespread?
Thanks!
The text was updated successfully, but these errors were encountered: