[Flow]: Add JS objects to Map.mergeIn & Map.mergeDeepIn #987

alex35mil · 2016-10-06T11:29:13Z

Adds vanilla JS objects to flow typings of Immutable.Map's mergeIn & mergeDeepIn.

facebook-github-bot · 2016-10-06T11:30:37Z

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status.

facebook-github-bot · 2016-10-06T11:44:25Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

wokalski · 2016-10-20T08:25:12Z

@alexfedoseev This repo is pretty much dead. I created a PR in flow-typed to add flow definitions for immutable there.

My fork is here. Feel free to submit a PR with your changes there.

lacker · 2016-12-02T19:17:53Z

Dead? Pshhh. It's not like we let this pull request sit for 15 months without responding.

Anyway my guess is that this has been abandoned. I was gonna ask for some sort of test if this was still alive, but I'm just gonna drop it. Sorry for letting this be quiet for so long :P and if you want to keep working on this please feel free to reopen!

wokalski · 2016-12-02T19:53:35Z

hey, thanks for bringing it back. I didn't mean to insult you. I just didn't see too much activity on any pull request at that time. Anyway, if you want a flow definition here (vs having one in flow-typed) I am more than happy to maintain it.

lacker · 2016-12-02T20:02:03Z

What's the "standard thing to do" around having a flow definition here vs in flow-typed? We should have a correct flow-typing as opposed to an incorrect one but I don't particularly have a stance on where it should go. Probably here? So I think it is awesome if you could help maintain this - so here's a question - do you think it makes sense when people submit a change to the flowtypes, to ask for a test that exercises the new type? I just want to make sure that we are protecting against stuff breaking as we improve things.

wokalski · 2016-12-02T20:14:06Z

In my opinion it's better when definitions are in the repo. For obvious reasons - it becomes immediately available to everyone who uses it. I'll ask in this PR for their point of view on it.

We might put it here for now and we will see how it evolves.

When it comes to testing, yes, I do think that tests are necessary. And tests are implemented here. I just need to make flow run on travis so that it's automated.

wokalski · 2016-12-02T20:15:49Z

Sorry that I spammed across few PRs to move people's work to my PR in flow-typed but I didn't know when and if our work here will ever be merged.

lacker · 2016-12-02T20:18:26Z

Yeah I understand, it's cool that you were spamming your alternative, I feel your pain man. But now that I am paying attention I would like the immutable.js repo to be all nice and things. I could really use your help reviewing Flow stuff though since I'm sort of a Flow noob myself. So what do you think we should do for this pull request - just accept it, or ask for some extra test type stuff, or what?

wokalski · 2016-12-02T20:34:19Z

I think that the order should be the following:

Decide on the blessed way to import things
Merge my PR I mentioned above which actually contains tests. It also needs some work but I can do it over the weekend.
Further improvements like this one

flow-typed makes a lot of sense because of versioning (separate versioning for flow versions and module versions). Also testing if the definitions actually make sense is a thing to look forward to. However for now, I'd like the "correct" definition to be anywhere and we can proceed with further improvements in the future.

wokalski · 2016-12-05T08:04:43Z

I made the tests run on travis in #878

alex35mil · 2016-12-05T09:59:19Z

@lacker @wokalski Hey guys, sorry for being quiet, I stopped visiting this PR after some time and missed the activity here as my gh became too noisy these days.

According to this comment serving shadow files w/ typings is still the way to go, so I guess this PR still makes sense.

AFAIU this PR is currently blocked by #878, let me know if I misread something and let me know what & when needs to be done here to get this merged. Thanks!

lacker · 2016-12-06T18:32:49Z

OK so #878 is in now so we have some Flow type refactorings and also you can now test flow types. Would you mind updating to merge those changes in, and also adding a test that catches these changes? Thanks!

alex35mil · 2016-12-07T09:10:27Z

@lacker Ok! I'll look into this on weekend 👍

alex35mil · 2016-12-12T03:10:14Z

@lacker @wokalski All done.

One note: it's not related to this PR, but when I run jest on my local branch, it throws: https://cl.ly/iPtb
Possibly similar issue: facebook/react-native#7802 (comment)

wokalski · 2016-12-12T03:54:27Z

I'm a bit confused. The diff shows that my changes are reapplied. Could you rebase the PR against current master? Possibly your fork's master is probably behind

alex35mil · 2016-12-12T04:17:13Z

I'm a bit confused. The diff shows that my changes are reapplied. Could you rebase the PR against current master? Possibly your fork's master is probably behind

@wokalski I think I rebased everything against master (flow tests weren't in my initial fork), let me check it one more time.

wokalski · 2016-12-12T04:31:35Z

If you look at this file and the contents of the PR, you can see that something is off. Maybe conflict resolution gone wrong?

wokalski · 2016-12-12T04:40:43Z

type-definitions/immutable.js.flow

+    mergeIn(
+      keyPath: ESIterable<any>,
+      ...iterables: (ESIterable<any> | { [key: string]: any })[]
+    ): Map<K,V>;


This makes me a bit confused. The mergeIn does change the type of the map, doesn't it? If so we (sadly) might need to turn V into any.

I used any b/c of this: http://facebook.github.io/immutable-js/docs/#/Map/merge

If any of the values provided to merge are not Iterable (would return false for Immutable.Iterable.isIterable) then they are deeply converted via Immutable.fromJS before being merged. However, if the value is an Iterable but includes non-iterable JS objects or arrays, those nested values will be preserved.

So if V is also Map or List, I can pass Object or Array as V and it still should be valid as it will be converted to Map or List by fromJS.

This part is correct. However in flow Map and List are parametrised (generic). So if you merge ESIterable<any> with Map<string, string> it becomes Map<string, any>, Map<string, Map<string | number>> would become Map<string, any>`.

AFAIK there's no way to overload the declaration for different V types so that we would have one for V: Map and one for V: List case.

Ok, so your suggestion is to turn those to this, right?

mergeIn( keyPath: ESIterable<any>, ...iterables: (ESIterable<any> | { [key: string]: any })[] ): Map<K,any>; mergeDeepIn( keyPath: ESIterable<any>, ...iterables: (ESIterable<any> | { [key: string]: any })[] ): Map<K,any>;

Yes, iff my reasoning is correct.

Also I had two further thoughts:

maybe the key path could be ESIterable<K> to make it type safe?

It should also be possible to merge it with a Map, not only an object, right?

Done.

maybe the key path could be ESIterable to make it type safe?

Agreed, done.

It should also be possible to merge it with a Map, not only an object, right?

Right, also w/ List etc., but I think it's handled by ESIterable<K>. Tests added.

wokalski · 2016-12-12T04:42:08Z

type-definitions/tests/immutable-flow.js

@@ -228,6 +228,9 @@ stringToNumber = Map({'a': 1}).removeIn([], 0)
 stringToNumber = Map({'a': 1}).mergeIn([], [])
 stringToNumber = Map({'a': 1}).mergeDeepIn([], [])

+anyMap = Map({'a': {}}).mergeIn(['a'], { b: 2 })
+anyMap = Map({'a': {}}).mergeDeepIn(['a'], { b: 2 })


Needs some tests for false positives. Put // $ExpectError on a line preceding a code which should output an error for these.

alex35mil · 2016-12-12T04:48:20Z

If you look at this file and the contents of the PR, you can see that something is off. Maybe conflict resolution gone wrong?

@wokalski That's weird, there was conflict only in dist/immutable.js.flow, which is generated file and it's regenerated anyway. Should I take contents of this file and just reapply my changes?

wokalski · 2016-12-12T04:50:07Z

It would be awesome.

alex35mil · 2016-12-12T04:54:41Z

@wokalski Just pasted contents and I see only my changes in diff: https://cl.ly/iPzf

wokalski · 2016-12-12T05:07:57Z

omg, sorry. This is what you call a 5 am review. I didn't notice that referred to dist/

alex35mil · 2016-12-12T05:15:08Z

@wokalski Np, I replied to comments 👍

alex35mil · 2016-12-14T11:54:38Z

@wokalski Comments addressed. Also I updated $ExpectError regex: now it's possible to add comments to it (example).

/cc: @lacker

wokalski · 2016-12-14T11:55:59Z

This is awesome. Great work 👍

Now it's up to somebody with commit access to merge it.

lacker · 2016-12-14T17:53:01Z

Thanks @alexfedoseev for submitting this and thanks @wokalski for reviewing!

facebook-github-bot added the CLA Signed label Oct 6, 2016

facebook-github-bot added the CLA Signed label Oct 21, 2016

lacker closed this Dec 2, 2016

lacker reopened this Dec 2, 2016

lacker added the flow label Dec 6, 2016

alex35mil force-pushed the add-js-objects-to-map-mergein-flowtypes branch from 71d7640 to bfe796e Compare December 12, 2016 02:38

Add JS objects to Map.mergeIn & Map.mergeDeepIn flowtypes

2d26e0c

alex35mil force-pushed the add-js-objects-to-map-mergein-flowtypes branch from bfe796e to 2d26e0c Compare December 12, 2016 02:45

wokalski reviewed Dec 12, 2016

View reviewed changes

Add tests for false positives

eab6a1a

alex35mil added 2 commits December 14, 2016 19:48

Make keypath type safe, turn value type into any

2afb113

Update regex: now it's possible to add comments to it

e9ba3e3

lacker merged commit 0bd996b into immutable-js:master Dec 14, 2016

[Flow]: Add JS objects to Map.mergeIn & Map.mergeDeepIn #987

[Flow]: Add JS objects to Map.mergeIn & Map.mergeDeepIn #987

Conversation

alex35mil commented Oct 6, 2016

facebook-github-bot commented Oct 6, 2016

facebook-github-bot commented Oct 6, 2016

wokalski commented Oct 20, 2016

lacker commented Dec 2, 2016

wokalski commented Dec 2, 2016

lacker commented Dec 2, 2016

wokalski commented Dec 2, 2016

wokalski commented Dec 2, 2016

lacker commented Dec 2, 2016

wokalski commented Dec 2, 2016 • edited Loading

wokalski commented Dec 5, 2016 • edited Loading

alex35mil commented Dec 5, 2016

lacker commented Dec 6, 2016

alex35mil commented Dec 7, 2016

alex35mil commented Dec 12, 2016

wokalski commented Dec 12, 2016

alex35mil commented Dec 12, 2016

wokalski commented Dec 12, 2016

Choose a reason for hiding this comment

alex35mil Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex35mil commented Dec 12, 2016

wokalski commented Dec 12, 2016

alex35mil commented Dec 12, 2016

wokalski commented Dec 12, 2016

alex35mil commented Dec 12, 2016

alex35mil commented Dec 14, 2016

wokalski commented Dec 14, 2016

lacker commented Dec 14, 2016

wokalski commented Dec 2, 2016 •

edited

Loading

wokalski commented Dec 5, 2016 •

edited

Loading

alex35mil Dec 12, 2016 •

edited

Loading