Implement function returning objects with properties that are included in first object but not included in second one.
Flow doesn't like generic Map<K, T> while iterating over properties, so I used Object.
Depends on D7752
Differential D7753
[lib] introduce function to find deep difference between two objects kamil on May 9 2023, 3:17 AM. Authored by Tags None Referenced Files
Details Implement function returning objects with properties that are included in first object but not included in second one. Flow doesn't like generic Map<K, T> while iterating over properties, so I used Object. Depends on D7752 Run tests
Diff Detail
Event TimelineComment Actions
Can you please provide more details on this issue? Object is basically any. Consider that ENG-3806 wouldn't have happened if Flow was able to realize that undefined is not a valid Object. Looking for:
Comment Actions Note that Map<K, T> defines the Map class in JS, and doesn't work for objects. If you absolutely must use an object, you should still be able to type it better, eg. { +[key: string]: mixed } or something
Comment Actions Map type is defined as follows in the codebase: type Map<K, T> = { +[key: K]: T }; Issues:
Issues 1, 2: using lodash which (according to flow-typed) allows generic key, e.g. _keys function, but this entire function should be based on only lodash utilities. Typing objects as { +[key: string]: mixed }:
I also tried different configurations with different generic types - same results.
I linked to the core.js definition, and I think the rest of problems is obvious. I don't think using Object (or even any) is a serious issue in this context. I feel like we trying to type some runtime things and it will never be accurate and always will require some casting, e.g. via any. Other function in this file also are based on Object and seems fine (findMaximumDepth or hash). Even if someone will call this function with some weird values, e.g. deepDiff(undefined, 3.14) the result will be {} which for me is suitable with a definition: returns an object with properties from obj1 not included in obj2. (there is a bug in this code that makes this function not return proper result for deepDiff([undefined, 4], 3.14) - but that's my oversight) Requesting review to see your point of view. Comment Actions
Ah, I got confused here... I think we should rename this type (see inline comment).
Thanks for explaining!! I think option 2 is the easiest to solve. It's a common issue in our codebase, and we have a solution in the very objects.js file you're testing here. (We basically solve it with an any-cast... sometimes those are necessary, but like unsafe blocks in Rust, we try to be very "targeted" with how we use those, and isolate it to reusable utilities instead of leaving it in business logic.) (By the way, that Flow issue has a GitHub issue, where this commit is mentioned, which solves the problem starting with Flow 0.196, which will be available in React Native 0.72 – currently in release candidate phase.)
I don't disagree. But without seeing everything you tried, it's hard for a reviewer to give advice. Sometimes there is an "easy" solution available that lets us type things better, and by pushing on this it might give you more context on how to resolve issues like this in the future, where it might be more of an issue.
Comment Actions Thanks for your response @ashoat. Adding the alternative solution I tried, using generics.
Comment Actions Thanks for annotating everything. I tried it locally and couldn't figure out anything better, unfortunately
Comment Actions Removing @tomek to unblock this because he does not work this week and his review is no longer applicable as this function was typed differently than in the previous version. |