Page MenuHomePhabricator

[lib] Fix useProcessAndSendDMOperation
AbandonedPublic

Authored by inka on Sep 11 2024, 3:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 12:59 PM
Unknown Object (File)
Tue, Nov 12, 2:28 PM
Unknown Object (File)
Tue, Nov 12, 2:22 PM
Unknown Object (File)
Sat, Nov 9, 8:01 AM
Unknown Object (File)
Thu, Nov 7, 7:10 PM
Unknown Object (File)
Fri, Nov 1, 5:37 AM
Unknown Object (File)
Sun, Oct 20, 7:46 PM
Unknown Object (File)
Sun, Oct 20, 7:45 PM
Subscribers

Details

Reviewers
tomek
kamil
Summary

This addresses ENG-9187, as well as fixes a more general issue with useProcessAndSendDMOperation:
If the callback from useProcessAndSendDMOperation was called twice in a continuous block of code, the second call would not see the effects of the first call. This is because underneath, dispatchActionPromise is called that changes the state. So for the second call to see the effects of the first call, a render has to happen between the calls.

Test Plan

Tested that when creatiing a new thick thread, the message appears to be in progress and then sent / failed.

Diff Detail

Repository
rCOMM Comm
Branch
inka/thread_ops
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Sep 11 2024, 3:28 AM
tomek requested changes to this revision.Sep 11 2024, 4:26 AM
tomek added inline comments.
lib/shared/dm-ops/process-dm-ops.js
315–319 ↗(On Diff #44033)

It would be better to use an object instead of a tuple so that the props are less confusing. Also, we shouldn't use any here - is there a way to avoid this? It seems like we're always calling the resolve without argument and never call the reject. Is it correct that we aren't rejecting at all?

323 ↗(On Diff #44033)

What is the benefit of introducing this function? Wouldn't it be simpler if we checked the queue length and early exited here?

This revision now requires changes to proceed.Sep 11 2024, 4:26 AM
lib/shared/dm-ops/process-dm-ops.js
315–319 ↗(On Diff #44033)

No idea why I used this odd type here... I'll fix it
I don't think we need reject.

323 ↗(On Diff #44033)

Effects cannot be asynchronous

tomek added inline comments.
lib/shared/dm-ops/process-dm-ops.js
315–318

We should prefer Array type, for mutable arrays. But this one should be immutable.

This revision is now accepted and ready to land.Sep 11 2024, 7:46 AM

Without this diff, problem from ENG-9187 is not resolved. However

  1. This diff introduces a different problem - if the component which calls this hook is unmounted before the effect is run, the operation is lost
  2. We are changing the approach to generating DM ops (D13290), and @kamil will be landing code rendering this diff unnecessary - most likely tomorrow

So we will land the rest of the stack. This doesn't break anything, because for now DM thread creation is hidden behind a flag anyway.