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

Add support for destructuring parameters + default values #183

Closed
TylerEich opened this issue Dec 10, 2014 · 49 comments
Closed

Add support for destructuring parameters + default values #183

TylerEich opened this issue Dec 10, 2014 · 49 comments
Assignees

Comments

@TylerEich
Copy link

I'm using ES6's new destructuring parameters with default values per the reminder from Brendan Eich (via 2ality.com).

So I would like to do this:

function rgba({ r, g, b, a = 1 }) {
    return [ r, g, b, a ];
}

Flow is throwing an Unexpected token = error at a = 1. Interestingly, JSHint has an issue with this pattern too.

@gabelevi gabelevi added the bug label Dec 11, 2014
@avikchaudhuri avikchaudhuri added incompleteness Something is missing and removed bug labels Jan 3, 2015
@rauchg
Copy link

rauchg commented Jan 20, 2015

+1

@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@cesarandreu
Copy link
Contributor

I have a question related to this issue: What's would be the expected behavior for typing params with default values?

My usage of default params tends to enforce strict types, e.g. this field is either 1 or some user-defined number, but it is always a number.

Considering the following:

/* @flow */
type params = { a: number }
function fn ({ a = 1 }: params): params {
  return { a }
}
fn({})

Would I get an error for not passing in an object that has a set to a number? It's optional, so I'm guessing it should probably be annotated as { a?: number } or { a: ?number }?

@rauchg
Copy link

rauchg commented Sep 21, 2015

It could also infer the type from the default value? Making a?: number unnecessary to write (perhaps more obscure).

@samwgoldman
Copy link
Member

@cesarandreu It would be { a?: number }, which is the same as { a: void | number }. ?number is the same as void | null | number. null is not a real type annotation in Flow (edit: it is now), but the point remains.

@cesarandreu
Copy link
Contributor

Thanks!

@samwgoldman
Copy link
Member

@eyyub You have some work on this. I looked over your branch and I think it makes sense to try to land the parsing support first, then the type checking support separately. If you don't have bandwidth to do this, would you mind if I tried to put a bow on what you have so far?

@eyyub
Copy link
Contributor

eyyub commented Oct 13, 2015

@samwgoldman Np I agree ! And yes sorry I don't have 'a bandwith' (nice expression :p) to do this actually, so do as you want and do not hesitate to ask me if something is weird on my branch !

@Arilas
Copy link

Arilas commented Dec 22, 2015

Any updates for this issue?

@AlexGalays
Copy link

I need diz !

Having to refactor

const { a, b = false } = obj

into

let { a, b } = obj;
if (b === undefined) b = false;

Just to please flow is very annoying :(

@trobertsonsf
Copy link

Would love to get a fix for this. Lack of support for such a nice language feature makes it feel like I'm getting handcuffed by flow =/

@samwgoldman
Copy link
Member

[Accidentally placed this comment in the wrong thread. Putting it here for posterity.]

I had a fair amount of WIP on this, but I tossed it out and haven't revisited.

My take is that this commit is too hacky and we shouldn't merge it. We need to separate parsing of objects and parsing of destructuring. Right now we do this weird thing where we parse an object, then transform it. Object syntax doesn't have defaults, so that method no longer works.

Ultimately this just confuses parsing support for assignment expressions ({ foo = bar } = obj). It's actually significantly simpler to just handle the case for params and variable declarations, which happens to be where destructuring+default is most often found.

I can still loop back on this in the coming weeks, but if someone is looking for something to do, please take it. If you do, I heartily recommend starting with the params/declaration cases and punting on assignment exprs. That's just me, though.

@0xR
Copy link

0xR commented Jan 10, 2016

This is currently the biggest missing es2015 feature. Not being able to write the full es2015 spec makes it difficult to consider moving to flowtype. Therefore I hope this is a high priority missing feature.

@bakape
Copy link

bakape commented Feb 3, 2016

Considering ES2015 parameter syntax is already implemented by all 3 major browsers, this is a sever limitation of Flow.

@samwgoldman
Copy link
Member

Yup, this has been on my plate for too long. I've been blocking this because I started working on it, then joined Facebook where I'm still getting used to things. Thanks for your patience. This feature is really high on my list of things to get out the door.

phanan pushed a commit to phanan-forks/vue that referenced this issue Jan 7, 2017
This commit uses ES2015 destructuring assignment to swap two variables
instead of using a tmp var. Flow has an open issue (facebook/flow#183)
with this feature, though, so // $FlowFixMe is used to temporarily
suppress the error.
@lleaff
Copy link

lleaff commented Apr 5, 2017

Flow fails to parse this valid ES2015 code:

var href = 'https://github.com/facebook/flow/issues/183'
var HREF_REGEXP = /^([^#?]*)(\?([^#]*))?(#(.+))?$/;
var [, path='', , query='', , hash=''] = HREF_REGEXP.exec(href) || [];
//                ^ Unexpected token ,

(flow.org/try)

The parsing error is suppressed if we put some dummy variables in the holes or if we remove the default assignments.

@nmn nmn added the parsing label Apr 5, 2017
@goldhand
Copy link

Shouldn't types from destructured params with default values be inferred from the default value as mentioned in @rauchg comment? It seems really redundant and verbose to do:

const flex = ({x = 'center', y = 'center'}: {x: string, y: string} = {}) => ({
  display: 'flex',
  justifyContent: x,
  alignItems: y,
});

seems like this should be enough:

const flex = ({x = 'center', y = 'center'} = {}) => ({
  display: 'flex',
  justifyContent: x,
  alignItems: y,
});

Sorry if there is already support for this already by my flow linter is lighting up

@Leksat
Copy link

Leksat commented Jun 22, 2017

Same as reported by @alexfedoseev, but with optional type: http://bit.ly/2rYnuYg

type params = { a?: ?number }
function fn ({ a = null }: params): params {
  return { a }
}
fn({})

@Jacse
Copy link

Jacse commented Jul 14, 2017

It's been more than two years since this issue was opened and it still doesn't work properly. What's the state of the issue? @samwgoldman did you improve on the partial fix?

@pascalduez
Copy link
Contributor

Hi,

wondering whether this is related to an issue I'm having with default rest parameters and union types:

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgAqAFngLZ4ByAhhWALxgDkAlgHZRxNgA+zAzgFcAxsLz9+3PkwTUATm3YBzKcwAm1NkrxymAbnTZ8RUnUYBvVGDAthcNgC4w-DHOUAaK2Dvw5Tl25angC+Bpi4BCTkeAAKcnA4DCbRvMkUNBRhRgRxCfxJltYYpngA-E5RFLk4IWFQgmzCGCz2YABicHAAFOZgxSmMrBxcYMFO1fwAlGCFYKjBQA

@lukemartin
Copy link

Flow fails with this very simple example of specifying a maybe type with a default null value:

const foo = ({name = null}:{name: ?string}) => {
	console.log(name);
}

1: const foo = ({name = null}:{name: ?string}) => {
             ^ null or undefined. This type is incompatible with
1: const foo = ({name = null}:{name: ?string}) => {
				^ string
1: const foo = ({name = null}:{name: ?string}) => {
						^ null. This type is incompatible with
1: const foo = ({name = null}:{name: ?string}) => {
				^ string

https://flow.org/try/#0MYewdgzgLgBAZiEMC8MAUBvMBDAtgUxRjAFcAbMgXwC4s99qYB+aAJwEswBzSgShQB8MDACgAkKEggy+AHRkQXNDgK8A3CMpA

@Rohit-L
Copy link

Rohit-L commented May 10, 2018

Is anybody from the flow team going to comment on this?

@simlu
Copy link

simlu commented Jun 20, 2018

We are running into the exact problem that @lukemartin describes. Would be great to get this fixed or at least have someone from the team comment on it.

@sibelius
Copy link

what is still failing?

@simlu
Copy link

simlu commented Jul 18, 2018

@sibelius lukemartin has provided a good example. Do you have problems understanding it?

@lukemartin
Copy link

hey @sibelius - this throws errors in 0.76.0:

const foo = ({name = null}:{name: ?string}) => {
	console.log(name);
}

https://flow.org/try/#0MYewdgzgLgBAZiEMC8MAUBvMBDAtgUxRjAFcAbMgXwC4s99qYB+aAJwEswBzSgShQB8MDACgAkKEggy+AHRkQXNDgK8A3CMpA

@Naddiseo
Copy link

Using $Subtype around the annotation seems to work, but I'm not sure if that's the right way to go in all situations.

const foo = ({name = null}: $Subtype<{name: ?string}>) => {
	console.log(name);
}

https://flow.org/try/#0MYewdgzgLgBAZiEMC8MAUBvMBDAtgUxRjAFcAbMgXwC4YASAZRICMoBPAB3wB4s99aAfmgAnAJZgA5pQB8AShQyYGAFABIUJBBl8AOjIhJaHATkBuFZRVA

@Buggytheclown
Copy link

const foo = ({name = true}: $Subtype<{name: ?string}>) => {
  //                 ^ flow does not check this
  console.log(name);
}

As proposed in SO

function foo(arg: {name: ?number}) {
  const { name = 42 } = arg;
  // the default value is only used for undefined values.
  console.log((name: (number | null)));
}

@simlu
Copy link

simlu commented Oct 19, 2018

@Buggytheclown Not a big fan of changing code to accommodate flow. In our scenario (jetbrains) this would cause us to lose introspection as well.

cjolowicz added a commit to cjolowicz/muckr-web that referenced this issue Jan 5, 2019
Flow mistakenly infers Maybe types when object destructuring is used
with default values (#183). As a workaround, we use just and
fromMaybe from utils.

  facebook/flow#183

Note that dotenv-webpack requires us to reference process.env
properties explicitly. The following will not work:

  export const API_URL = env('API_URL')
@tomscallon
Copy link

tomscallon commented Feb 25, 2019

Perhaps a separate issue, but I'm having a very similar problem with non-maybe types. I'm trying:

/* @flow */

type P = string | Array<string>;

function p(p: P = []) {}

type O = {p: P};

function o({p = []}: O) {}

And I get the following errors:

9: function o({p = []}: O) {}
               ^ array type [1] is incompatible with string [1].
References:
7: type O = {p: P};
                ^ [1]
9: function o({p = []}: O) {}
                   ^ empty array literal [1] is incompatible with string [2].
References:
9: function o({p = []}: O) {}
                   ^ [1]
7: type O = {p: P};
                ^ [2]

Interestingly, the errors go away if I simplify the type P to Array<string>. It's almost like Flow is expecting my default value to be all of the unioned types, rather than just one of them.

facebook-github-bot pushed a commit that referenced this issue Jul 15, 2019
Summary:
Bindings introduced by destructuring a type annotation should behave as
annotations themselves. That is, `p` in `var {p}: {p: T}` should constrain any
subsequent write to be a subtype of `T`.

This logic works today by wrapping the binding in an AnnotT using BecomeT
internally, which works for most cases. However, since BecomeT performs
unification, it is necessary that its lower bound be resolve once and exactly
once.

The once-and-exactly-once invariant is broken by union-like types when combined
with the destructuring operation. Consider the following example:

```
var {p}: {p:string}|{p:number} = ...
```

When evaluating this variable declaration, we emit the following constraints:

1. {p:string}|{p:number} -> GetPropT ('p', T)
2. T -> BecomeT T'
3. P = T'

Constraint (1) is processed by splitting the lower bound, which turns into two
separate constraints `string -> T` and `number -> T`. Since `T` resolves twice,
the `BecomeT` constraint misbehaves and we see a spurious error that `number` is
not compatible with `string`.

This diff deals with the above by handling union-like types specially in the
`DestructuringT` use type.

Fixes #183
Fixes #2198
Fixes #2220
Fixes #4077
Fixes #4270
Fixes #5461
Fixes #5745
Fixes #6408

Reviewed By: panagosg7

Differential Revision: D15457398

fbshipit-source-id: 22c9aba1e1df475c73b36a92bdf7ff5cf57504a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests