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

[lodash_v4.x.x] lodash.values generic parameters #3005

Closed
wants to merge 5 commits into from
Closed

[lodash_v4.x.x] lodash.values generic parameters #3005

wants to merge 5 commits into from

Conversation

beaucollins
Copy link
Contributor

@beaucollins beaucollins commented Dec 13, 2018

The existing values definition is:

    values(object?: ?Object): Array<any>;

So if you use an Object with an indexer definition, the type gets erased when using lodash.values

type Ship = {| name: string, type: string |};
type ShipIndex = { [key: string]: Ship };

const ships = { 'x-wing': { name: 'X Wing' , type: 'Fighter' }};

values(ships); // Array<any>; 😢

By adding an overload on values that accepts indexed types we get to keep the correct type:

values(ships); // Array<Ship>; 🎉

@AndrewSouthpaw
Copy link
Contributor

Thanks for the PR! Currently your build is failing, and we'll need that to pass to merge it in. You can check out the build in Travis, or read about running libdef tests locally in CONTRIBUTING.md.

@beaucollins
Copy link
Contributor Author

beaucollins commented Dec 19, 2018

Locally I'm seeing this:

flow-typed (master) $ ./build_and_test_cli.sh 
// noise noise noise
All libdefs are named and structured correctly. (Found 755)

@beaucollins
Copy link
Contributor Author

beaucollins commented Dec 19, 2018

@AndrewSouthpaw

Just to confirm, these failures already exist in f0bbb7e

$> cd cli
$> git checkout origin/master
HEAD is now at f0bbb7e5 Added type definitions for 'tape-cup' package (v4.7.x) (#3015)
$> echo "// lol" >>  ../definitions/npm/lodash_v4.x.x/flow_v0.63.x-/lodash_v4.x.x.js 
$> node dist/cli.js run-tests --onlyChanged

And voila:

Fetching all Flow binaries...

NOISE NOISE NOISE NOISE NOISE    
    
    Found 10 errors and 14 warnings
    

So I at least didn't increase any errors or warnings: https://travis-ci.org/flow-typed/flow-typed/jobs/470139319#L1237

@beaucollins beaucollins changed the title lodash.values generic parameters [lodash_v4.x.x] lodash.values generic parameters Dec 19, 2018
@beaucollins
Copy link
Contributor Author

Tests will pass once #3020 lands.

@beaucollins
Copy link
Contributor Author

@AndrewSouthpaw test failures were fixed in #3020 so this is passing now.

I also added a test for the new type definition in 81d6c2d.

@mhsdef
Copy link

mhsdef commented Jan 5, 2019

@villesau is this ok to go?

@villesau
Copy link
Member

villesau commented Jan 6, 2019

@beaucollins I think this is a bit problematic, for the same reasons why native .values returns mixed. See e.g facebook/flow#2221 (comment) and facebook/flow#2221 (comment)

For exact objects it's a different story though, there all possible values are known beforehand.

@villesau villesau self-assigned this Aug 9, 2019
@gantoine gantoine closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants