Part of ENG-3966
This is a controversial diff, the approach is discussed in comments.
Detailed description and all TODO follow-ups are tracked in ENG-4285. As soon as RN / Flow is upgraded, these should be reverted.
Depends on D8370
Details
Details
This stack is tested altogether, CI will fail at this point due to Flow breaking changes: Some Flow issues are worked around by this diff, remaining ones are fixed in D8373
Actual test plan in the last diff.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/input/input-state-container.react.js | ||
|---|---|---|
| 977 ↗ | (On Diff #28256) | Reducer accepts both blobURI or holder so we can now use the new field here when dispatching update |
Comment Actions
This is a lot more anys and $FlowFixMes than I expected, and they aren't being immediately cast to a proper type after the any-cast. I'll need to take a closer look and try to type it myself before I can accept this, as I suspect there are better ways to do it. I'm also not sure that every single any-cast and $FlowFixMe is related to the issue that will be solved in the updated version of Flow
Comment Actions
Also it looks like this fails Flow CI
Ah I guess this is intentional? I'm having a hard time following this diff stack...
I suspect it would have been less confusing / complicated to simply appease Flow by destructuring where necessary
Comment Actions
I am behind on a bunch of things and don't have the time to investigate this further right now, but I strongly suspect that we can do better here
A good place to start would be to apply my suggestions in D8370. After that, the main goal would be to significantly reduce the use of any or $FlowFixMe. Like unsafe blocks in Rust, these should be extremely rare, and if you have a strong need for one, you can ideally limit the use of any or $FlowFixMe to one specific place, and immediately cost the result to something else.
@bartek – if you're still stuff following that, let me know and I'll patch this locally and take a look
Comment Actions
I didn't mention this in the test plan, I probably should. This one fixes workarounds some Flow issues, but not all. The remaining ones are fixed by D8373
Comment Actions
Rebased on applied feedback in parent diff, unfortunately no meaningful improvements here.
I was able to limit it to 3 places right now. It's one place if excluded the test file - see inline comment
| lib/reducers/message-reducer.test.js | ||
|---|---|---|
| 69–71 ↗ | (On Diff #28329) | These are still failing with: │ Cannot call `reduceMessageStore` with `updateMultiMediaMessageMediaAction` bound to `action` because property `localMediaSelection` is missing in `Shape` [1] but exists in object literal [2] in property `payload.mediaUpdate`. Flow (incompatible-call) [70, 5] Flow completely cannot understand Shape<Media> properly. When removed the localMediaSelection, it started the same with every other prop.erty. I'm afraid we need Flow 0.203 to fix this (0.202 doesn't work): playground link |
Comment Actions
Thanks for pointing to Shape<Media>. I patched this locally and found we can improve things a little bit after fixing that type.
I got it down from 6 $FlowFixMes and 1 any to just 2 $FlowFixMes using this patch: https://gist.github.com/Ashoat/d83301775bc01c7f71af3350ce440dbb
Note that I was working off a custom branch, since D8374 hasn't been rebased on top of the latest version of this diff. I ran yarn arcpatch D8374 and then tried to apply the changes in the most recent revision of this diff, but it's possible I missed something. Let me know if it doesn't work in your branch.
Comment Actions
Thank you for doing this! Splitting the Shape<Media> into a union of shapes is a clever trick!
Note that I was working off a custom branch, since D8374 hasn't been rebased on top of the latest version of this diff. I ran yarn arcpatch D8374 and then tried to apply the changes in the most recent revision of this diff, but it's possible I missed something. Let me know if it doesn't work in your branch.
Ah, I should have rebased the remaining diffs too.
I'll apply your patch and fix/tweak some comments