Page MenuHomePhabricator

[web] Move `deleteThreadAction` up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic

Authored by atul on Apr 19 2022, 4:02 PM.
Tags
None
Referenced Files
F3173328: D3783.diff
Thu, Nov 7, 3:10 PM
Unknown Object (File)
Mon, Nov 4, 1:40 PM
Unknown Object (File)
Tue, Oct 22, 11:15 AM
Unknown Object (File)
Tue, Oct 22, 11:15 AM
Unknown Object (File)
Tue, Oct 22, 11:15 AM
Unknown Object (File)
Tue, Oct 22, 2:05 AM
Unknown Object (File)
Mon, Oct 21, 11:37 PM
Unknown Object (File)
Sun, Oct 13, 3:33 AM

Details

Summary

Move deleteThreadAction out from inner class component to outer "Connected" functional component.

As part of the work to refactor ThreadSettingsModal... specifically to turn it into a functional component


Depends on D3770

Test Plan
  1. Open ThreadSettingsModal
  2. Go to Delete tab
  3. Enter password and delete thread
  4. Ensure that things look/behave as expected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/modals/threads/thread-settings-modal.react.js
465–466 ↗(On Diff #11652)

Yes, I know we consider leaving TODO comments in the code an anti-pattern, but I think it's the best way forward in this case.

The "ref" is currently "within" the class component and can't be accessed from the outer functional component.


"Why not pull the input refs out from the class component first then?"

I tried sequencing the "lifting of refs" before this and found it very frustrating... particularly with regard to flow issues.

I think the best bet is to leave a reminder here that we want to focus that input, and then once we've "turned ThreadSettingsModal into a functional component," we can introduce replacement refs with React.useRef and bring back this focus behavior.

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 19 2022, 4:08 PM
Harbormaster failed remote builds in B8342: Diff 11652!
atul requested review of this revision.Apr 19 2022, 4:13 PM
tomek added inline comments.
web/modals/threads/thread-settings-modal.react.js
456 ↗(On Diff #11652)

@def-au1t in his stack introduces a new method popModal that should be used here. The method clearModal will not be defined after that stack so there's no risk of forgetting about the update here.

465–466 ↗(On Diff #11652)

Thanks for the explanation - really like the fact that it explains what's going on and why.

Do you think it is a good idea to create a linear task for it?

469 ↗(On Diff #11652)

Can we modify the code so that the callback depends only on thread.id?

This revision is now accepted and ready to land.Apr 21 2022, 3:18 AM
atul added inline comments.
web/modals/threads/thread-settings-modal.react.js
456 ↗(On Diff #11652)

Ah thanks for the heads up. It looks that stack requires revision before it can be landed.

I think the best bet is for me to

  1. Land this diff as is
  2. Commandeer D3665
  3. Rebase and make the necessary changes in ThreadSettingsModal
  4. Hand the revision back to Jacek

That way I should be able to move forward with the ThreadSettingsModal refactor without breaking Jacek's existing work?

cc @def-au1t

465–466 ↗(On Diff #11652)

Do you think it is a good idea to create a linear task for it?

Sure, don't think it would hurt: https://linear.app/comm/issue/ENG-1028/[web]-reintroduce-focusing-of-inputs-in-threadsettingsmodal

469 ↗(On Diff #11652)

Yup, will make that change

atul marked 2 inline comments as done.

rebase before landing

web/modals/threads/thread-settings-modal.react.js
469 ↗(On Diff #11652)

Hm, when I modify the deplist I get the following:

React Hook React.useCallback has a missing dependency: 'threadInfo'. Either include it or remove the dependency array.


Going to land as is, open to making any changes in a subsequent diff if necessary

web/modals/threads/thread-settings-modal.react.js
456 ↗(On Diff #11652)

Up to you, but usually the diff author should be responsible for rebasing and resolving conflicts. It's a part of the process that we're introducing conflicting changes.

web/modals/threads/thread-settings-modal.react.js
469 ↗(On Diff #11652)

That's due to invariant using threadInfo. We can change that and use invariant(threadInfo.id, ... or, if that doesn't work, introduce a new variable threadID before the callback and use it as a dependency and in the invariant.

Overall, it is a bit wasteful to create a new callback every time a threadInfo changes, when we only use id part of it. This improvement should be introduced, but may be easier when invariants are gone.

web/modals/threads/thread-settings-modal.react.js
469 ↗(On Diff #11652)

Makes sense, fwiw I think I remember trying to change the invariant to be threadInfo.id and a few other approaches but still ran into the issue


Regardless, the invariant in deleteThreadAction is removed altogether as of D3837

web/modals/threads/thread-settings-modal.react.js
469 ↗(On Diff #11652)

Sometimes that happens, in which case you can just pull threadInfo.id out into its own variable outside of the block