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

When using Object as maps, Object.values treats values as mixed #2221

Closed
xtinec opened this issue Aug 9, 2016 · 68 comments
Closed

When using Object as maps, Object.values treats values as mixed #2221

xtinec opened this issue Aug 9, 2016 · 68 comments
Assignees

Comments

@xtinec
Copy link

xtinec commented Aug 9, 2016

I'm using objects as maps in the example below. It appears the type of Object.values is an array of mixed as opposed to an array of the Point type I defined.

type Point = {
  x: number,
  y: number,
};
type PointMap = { [key:string]: Point }

const foo: PointMap = { "1": { x: 5, y: 6}, "2": { x: 7, y: 8} };

Object.values(foo).map(p => {
  return p.x;
})

It appears if I do use Object.keys instead, flow can correctly infer the type of foo[key]

Object.keys(foo).map(key => {
  return foo[key].x;
})

This seems like a bug in flow, as we use Object as a map here and the type checker has enough info to correctly infer the type of Object.values(). Yes?

@gabelevi
Copy link
Contributor

Why does Object.values return Array<mixed> currently?

At the moment, if you have a variable obj with the type { foo: string }, what does that mean? It means that obj.foo has the type string. But it doesn't mean that obj only has a single property. There may be other properties. This is because we let you write code like this:

function foo(obj: { foo: string, bar: number }) : { foo: string } {
  return obj;
}

Allowing subtypes to have fewer properties than super types makes structural subtyping with object types work well. But it means when you write Object.values(obj), we can't say with certainty what types are in the resulting array.

How do I specify an object with ONLY certain properties?

As of v0.30.0, there is no way. However, @bhosmer is here to save the day! He's in the middle of landing the $Exact magic type. $Exact<{foo: string}> is the type of an object who has exactly one property named foo that is a string.

How does this help with Object.values?

Building on top of this, @samwgoldman is updating the types of Object.{entries,keys,values} to offer better types when used with exact types. So Object.keys(obj) should return Array<'foo'> when obj has the type $Exact<{foo: string}>. This might make v0.31.0, but if it doesn't it will definitely make v0.32.0.

@taion
Copy link

taion commented Oct 27, 2016

I think this and #2174 are duplicates.

@gabelevi Does what you're saying apply for map types, though? It seems like maps don't have this issue? As in the following gives me a type error on a:

const a: {[key: string]: string} = {b: 3};
const b: {a?: string} = {b: 3};

Could this just be a matter of adding an overload for the case where the argument to Object.values or Object.entries is a map type?

@dsimmons
Copy link

Having this exact same problem right now.

Use case:

// types.js
export type FieldLabel =
    'SOME_FIRST_LABEL'
  | 'SOME_SECOND_LABEL'
  // ...
;

// constants.js
import type { FieldLabel } from './types';

export type FieldLabels = { [key: FieldLabel]: FieldLabel };
export const FIELD_LABELS: FieldLabels = Object.assign({}, ...[
  'SOME_FIRST_LABEL',
  'SOME_SECOND_LABEL',
  // ....
].map(label => ({ [label]: label })));


// elswhere, someFile.js
import type { FieldLabel } from '../types';

import { FIELD_LABELS } from '../constants';

const fieldLabels: Array<FieldLabel> = Object.values(FIELD_LABELS).sort();

Results in: string enum This type is incompatible with (:0:1,0) mixed (/private/tmp/flow/flowlib_28d43509/core.js:52:39,43) [javascript/flow]

@mull
Copy link

mull commented Feb 22, 2017

@verkholantsev
Copy link

verkholantsev commented Mar 29, 2017

When I try to use exact notation to map

/* @flow */

const a: {|[string]: string|} = {'a': '1'};
const values: Array<string> = Object.values(a);

I get these errors

3: const a: {|[string]: string|} = {'a': '1'};
                                   ^ property `a`. Property not found in
3: const a: {|[string]: string|} = {'a': '1'};
            ^ object type
4: const values: Array<string> = Object.values(a);
                       ^ string. This type is incompatible with
[LIB] static/v0.42.0/flowlib/core.js:52:     static values(object: any): Array<mixed>;
                                                                               ^ mixed

Can you please tell me

  1. Why does exact notation not working with Object.values in this case?
  2. Why is it not possible to declare some variable as object map and then assign an object literal to it?

@vkurchatkin
Copy link
Contributor

Type declarations like {|[string]: string|} are not really useful. Exact implies that all properties are known in advance.

@verkholantsev
Copy link

@vkurchatkin Yes, that makes sense. But what how to deal with Object.values returning Array<mixed> for {[string]: string}?

@villesau
Copy link
Contributor

villesau commented May 23, 2017

How the below should be written? Even when everything is exact, that doesn't work with 0.45.0

type Thing = {|name: string, b: number|};
type Things = {|[id: string]: Thing|};
const getSortedArray = (things : Things) => Object.values(things).sort((a, b) => a.name.localeCompare(b.name));

what we get:

216: const getSortedArray = (things : Things) => Object.values(things).sort((a, b) => a.name.localeCompare(b.name));
                                                                                                             ^^^^ property `name`. Property cannot be accessed on
216: const getSortedArray = (things : Things) => Object.values(things).sort((a, b) => a.name.localeCompare(b.name));
                                                                                                           ^ mixed

This is causing headache too often.

@madole
Copy link

madole commented Aug 16, 2017

Any solution for this? Running into this all afternoon :(

@aikar
Copy link

aikar commented Aug 21, 2017

Just hit this too :(

for (const [key, value] of Object.entries(obj)) { } is a very common code pattern that's really messed up by this.

I've resorted to disabling flow on affected lines :(

@jcready
Copy link
Contributor

jcready commented Aug 21, 2017

The reason this isn't supported was explained pretty simply by @vkurchatkin in my (now closed) PR:

Here is a simple example:

function test(val: { foo: string }) {
  Object.values(val).forEach(v => {
  	v.toLowerCase();
  });
}

test({ foo: 'x', bar: 123 });

This going to pass typechecking, but fail at runtime

My PR would've set the following function type signatures for Object.entries, Object.values, and Object.keys, but was closed after it was pointed out how unsound these are:

-    static entries(object: any): Array<[string, mixed]>;
+    static entries <T> (o: { +[s: string]: T }) : Array<[string, T]>;
+    static entries <T> (o: Array<T> | $ReadOnlyArray<T>) : Array<[string, T]>;
+    static entries (o: any) : Array<[string, mixed]>;

+    static keys <T> (o: { +[key: T]: any }): Array<T>;
     static keys(o: any): Array<string>;

-    static values(object: any): Array<mixed>;
+    static values <T> (o: { +[s: string]: T }) : Array<T>;
+    static values <T> (o: Array<T> | $ReadOnlyArray<T>) : Array<T>;
+    static values(o: any): Array<mixed>;

@villesau
Copy link
Contributor

villesau commented Aug 22, 2017

@jcready that's not the case with exact types where we know exactly what the object contains. see my comment above #2221 (comment)

It's not been only once or twice when someone in our dev team has spent way too much time figuring out what's wrong with their flow typings when this has been the reason. supporting exact types would help a bit.

@TiddoLangerak
Copy link
Contributor

TiddoLangerak commented Aug 22, 2017

I think the above definitions are almost correct, but the object-as-map version should be replaced with this:

static values<T, O : { +[string] : T }>(val : $Exact<O>) : Array<T>;

(and likewise for keys and entries).

I'd love to provide a patch myself, but I currently don't have time to get the flow build working. I might give it a shot later this week, if no-one else took it before me.

EDIT: this definition is also incorrect, as @jcready pointed out

@asolove
Copy link
Contributor

asolove commented Aug 22, 2017

In a comment on another issue, @samwgoldman mentioned that support for better object-as-dictionary typings on exact objects was already his top priority. I think this issue might be closable since his work on that is already underway.

@aikar
Copy link

aikar commented Aug 22, 2017

Let's close it when it's actually fixed :P

@jcready
Copy link
Contributor

jcready commented Aug 22, 2017

@villesau @TiddoLangerak the problem is that indexers in exact object types don't work as you'd expect (at least right now):

declare class Obj {
  static values <T> (o: {| +[s: string]: T |}) : Array<T>; // exact object w/ indexer
}

function test(val: {| foo: string |}) { // exact object w/o indexer
  Obj.values(val).forEach((v) => {
    v.toLowerCase();
  });
}

test({ foo: 'bar' });

This results in the following error:

6:   Obj.values(val).forEach((v) => {
                ^ property `foo`. Property not found in
2:   static values <T> (o: {| +[s: string]: T |}) : Array<T>;
                           ^ object type

Alternatively if we use @TiddoLangerak's definition for values:

static values<T, O : { +[string] : T }>(val : $Exact<O>) : Array<T>;

Then type-at-pos reports that the v parameter inside the forEach function is (unknown).

Unrelated, but @asolove I really wish the flow team would stop closing issues before they've actually fixed the issue. There seems to be an odd treatment of issues in this repo where they will be closed when the issue has simply been triaged or someone says they are going to make said issue a priority. Issues should be referenced in commit/PR messages so they can be closed automatically when the code actually gets merged to master.

@TiddoLangerak
Copy link
Contributor

You're right, I completely missed that v became untyped 😞

@underoot
Copy link

Seems that problem actual now

@adek05
Copy link

adek05 commented Jan 10, 2018

Is anyone looking at resolving this?

@a-tokyo
Copy link

a-tokyo commented Jan 11, 2018

Still facing the problem with 0.61.0

@allmaxgit
Copy link

Actual in 0.63.1

@sahrens
Copy link

sahrens commented Feb 13, 2018

@gabelevi: Any progress on this when working with exact types now that they are a thing? It is quite a ways since v0.32:

So Object.keys(obj) should return Array<'foo'> when obj has the type $Exact<{foo: string}>. This might make v0.31.0, but if it doesn't it will definitely make v0.32.0.

@FireyFly
Copy link
Contributor

It makes sense to use ES6 Map objects for when you want a mutable map, rather than using objects for it. But I think it's fair to say that oldschool "objects-as-maps" serve some purpose still--for instance in APIs where an object is provided as a literal mapping keys to values, like combineReducers in Redux. Using plain objects for the state tree in Redux (and other similar cases) is another example.

The original issue is very old, and plenty of things have been added to Flow since... wouldn't it make for a modern libdef for Object.values to type it as something like <T: {}>(object: T) => Array<$Values<T>> (together with a declaration like the current one probably, to catch non-object cases)? After all, the documentation for $Values<T> states that it "represents the union type of all the value types of the enumerable properties in an Object Type T.", which I think is exactly what we want to model.

That said, the current implementation of $Values<T> seems pretty janky. I tried something like this:

declare class Object {
  static values<T: {}>(object: T): Array<$Values<T>>;
  static values(object: mixed): Array<mixed>;
}

declare var test: {
  name: string,
  value: number,
};

const v = Object.values(x);
// type-at-pos says v has type Array<$Values<{name: string, value: number}>>

const w: Array<string | number> = v;
// produces: Cannot assign v to w because:
//  • $Values [1] is incompatible with string [2] in array element.
//  • $Values [1] is incompatible with number [3] in array element.

Any eventual jankiness with $Values<T> should probably be sorted in a separate issue, but I hope we can improve the libdef for Object.values since it's rather hard to use at all currently... and I think it's important for the core libdef to model JS core as well as possible (whilst staying sound).

@goodmind
Copy link
Contributor

@FireyFly This isn't sound

class A {
  x: number;
  constructor(x: number) {
    this.x = x;
  }
  f() {
    return this.x;
  }
}

type O = {+x: number, +f: () => number};
const test: O = new A(2);

((2): $Values<O>); // own property
((() => 2): $Values<O>); // not own property

console.log(Object.values(test)); // [2]

@FireyFly
Copy link
Contributor

Oh hmm... right, that's a good point, my bad. I guess the iterating over all properties rather than only own properties means we can't really do any better for a type for Object.values then. :\

@lxe
Copy link

lxe commented May 14, 2019

For sealed objects, which are very-well inferable, values/keys/entries should return sealed arrays, which also seem inferable.

@laszukdawid
Copy link

Nice. Was promised a better way and burden-free development, but seems like another day with more problems of trying to incorporate the Flow into a new project.

Is anyone actually looking at this to provide some solutions or even, dare to say it, fix this? Today is the third anniversary of this issue.

@jcready
Copy link
Contributor

jcready commented Aug 10, 2019

@laszukdawid yes, @goodmind has made sound versions of values/entries, but apparently this still isn't good enough for the flow maintainers.

@lyleunderwood
Copy link
Contributor

@jcready That's not a very generous interpretation. Why not jump on discord and discuss it with them?

@jcready
Copy link
Contributor

jcready commented Aug 11, 2019

@lyleunderwood what would you consider a more generous interpretation? I asked if I should make a PR for @goodmind's proposed libdef changes. @goodmind said:

@dsainati1 doesn't like idea of exact constraint.

[...] this was from Discord months ago when I proposed this libdef change

I attempted to find the conversation on discord and I believe this is it. @dsainati1's argument against it was:

the whole concept of a supertype of all exact objects is inherently flawed though, since the point of exact objects is that they don't exhibit width subtyping

My interpretation of this is basically that even though @goodmind's implementation using an exact constraint is sound in practice (or at least as sound as the current Object.keys() implementation), in theory it just doesn't feel right to @dsainati1.

The most reasonable argument against the exact constraint implementation I've see was from @samwgoldman in this comment from January 2018:

Vladimir is right. There are two issues.

Width subtyping:

var o: { p: number, q: string } = { p: 0, q: "", foo: "foo" };
var keys: "p" | "q" = Object.keys(o); // lies
var vals: number | string = Object.values(o); // lies

Proto vs. own

var o: { p: number, q: string } = Object.create({ p: 0, q: "" });
var keys: Array<"p" | "q"> = Object.keys(o); // lies
var vals: Array<number | string)> = Object.values(o); // lies

Exact object types are great for the former case, because they by definition exclude values having more properties. For the latter case, we really need to provide better mechanisms to specify object layout as part of types, since so many APIs depend on it. It's on our radar.

And while I agree that the proto vs. own issue still isn't solved by @goodmind's exact constraint implementation, the fact is flow has this same issue with Object.keys() right now. I'll quote myself here:

So if this concession is made for Object.keys(), why not also for Object.values() or Object.entries()?

@lyleunderwood
Copy link
Contributor

The impression I got was that flow maintainers didn't want to make an API knowing they'll just have to change the API again down the road when the theoretically correct solution eventually comes along.

For what its worth, if I could override the builtin libdef with @goodmind's exact constraint, I would have by now.

@yangshun
Copy link

yangshun commented May 10, 2020

Here's a workaround that uses Object.keys() instead of Object.values():

type Line = { name: string };
type Table = { [id: string]: Line };
const table: Table = { 'random': { name: 'Test' } };

// Error.
Object.values(table).forEach((line: Line) => console.log(line.name));
// No error.
Object.keys(table).map(key => table[key])
  .forEach((line: Line) => console.log(line.name));

Try it out on Flow playground

@danielo515
Copy link

The object.keys workaround is an old a know one, but I think that not using some of the best ES6 features because flow it's a bad thing to face.
I think this should be prioritised since it has been open for more than 3 years.

@EgMan
Copy link

EgMan commented Jun 18, 2020

+1 on this issue. Still using the workaround.

@ealemda2
Copy link

ealemda2 commented Jul 1, 2020

Closing in on 4 years on this issue. Any updates?

It really is an inconvenience when you spend 2 minutes writing a reduce function on Object.values() and then another hour on trying to write it in a way that flow would allow when you know exactly what .values() should return. On top of that everyone on your team will ask for an explanation why you had to write it the way you did (see all the workaround ideas above), because it is counter-intuitive/ looks like bad practice (e.g. the any solution).

@edoardo-bluframe
Copy link

Hey guys!

Is cast to : any really our best solution?

Can we do better than that? 😄

@EgMan
Copy link

EgMan commented Aug 9, 2020

🎉 Happy Birthday!! 🥳🎂

@mmkvdev
Copy link

mmkvdev commented Feb 1, 2021

Check for the type of the Object. In my case I accidentally set the type of the variable as Array<Object> but instead it should just be an Object.

If there's any condition associated inside the map function, we need to return some JSX for the callback or else null.

It's always advisable to use a variable outside the parent return to capture the results from a function (return value of Object.keys((callback))) and then use that variable to render inside the parent JSX to avoid flow from catching issues like:

  • Array.prototype.map() expects a value to be returned at the end of arrow function
  • Expected to return a value at the end of arrow function.

@nnmrts
Copy link
Contributor

nnmrts commented Sep 2, 2021

I think the assumption that solving this issue does not "align with [their] priorities" is safe. Or "sound", as the Flow team at Facebook would say. Let's close it. :)

https://medium.com/flow-type/clarity-on-flows-direction-and-open-source-engagement-e721a4eb4d8b

@FezVrasta
Copy link
Contributor

Is there any chance for this issue to be resolved?

facebook-github-bot pushed a commit that referenced this issue Dec 5, 2022
…ictionary (mixed otherwise)

Summary:
User request from Dec 2021, May 2021, Dec 2020, Apr 2020

For `Object.entries`, keys is typed exactly like it is for `Object.keys`

For both, values is typed as such:
- Dictionaries: the value of the dictionary
- Objects & Instances: `mixed` - this avoids created non-performant union types

Error diff analysis:
- Previously, `Object.entries` returned `string` for the key type, rather than having the same behavior as `Object.keys`. We unsafely allow property read/writes with `string`.
- Error code for suppressions has changed in some places from `incompatible-call` to `incompatible-use`
- Many errors have simply moved (previously error on usage of `mixed`, now erroring on still invalid but more accurate type)

After WWW diff D41527152, errors are only around ~500.

Alternative 1 was to create a new utility type `$DictValues` and type the `Object.values` and `Object.entries` lib defs with this. Cons of this approach are introducing a new lib def, and the fact that `Object.entries` keys definition would differ from `Object.keys`, since `$Keys` and `Object.keys` are implemented differently.

Changelog: [errors] Instead of `mixed`, type the result of `Object.values` and `Object.entries` on a dictionary to be the dictionary values, and `Object.entries` keys to behave like `Object.keys`

Closes #2174, #2221, #4771, #4997, #5838

Reviewed By: jbrown215

Differential Revision: D35710131

fbshipit-source-id: 3163bc02dfc7cb70a5d6c0ba7da574a793672421
@gkz
Copy link
Member

gkz commented Dec 5, 2022

With above commit, object types that have an indexer and no explicit properties (e.g. {[string]: number}) have the type of their dictionary values returned by Object.values (e.g. Array<number>), and similarly with Object.entries, except the first element of the tuple works exactly like Object.keys (in this case, Array<[string, number]>)

@gkz gkz closed this as completed Dec 5, 2022
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