Page MenuHomePhabricator

[web] Add a selector for fetching picked community in Chat tab
AbandonedPublic

Authored by inka on Mar 7 2023, 3:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 4, 10:10 PM
Unknown Object (File)
Thu, Apr 4, 10:09 PM
Unknown Object (File)
Thu, Apr 4, 10:02 PM
Unknown Object (File)
Tue, Apr 2, 9:25 AM
Unknown Object (File)
Feb 21 2024, 12:32 AM
Unknown Object (File)
Feb 21 2024, 12:32 AM
Unknown Object (File)
Feb 21 2024, 12:31 AM
Unknown Object (File)
Feb 20 2024, 10:53 PM
Subscribers

Details

Reviewers
kamil
tomek
michal
Summary

Adding a selector that fetches the id of the community currently picked in Chat tab
issue: https://linear.app/comm/issue/ENG-3176/make-pressing-a-drawer-item-in-chat-tab-filter-available-chats

Test Plan

Called the selector, and checked that it returns the correct value

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Mar 7 2023, 3:19 AM
web/selectors/thread-selectors.js
111

I'm not a big fan of creating such simple hooks - feels like it adds a useless boilerplate to the code.

It can easily be done as a simple selector, and it's used only once looking through this stack, but I think I've seen one-line hooks like this in the codebase, so probably it's okay 😄

On the other hand in D6974 (ChatDrawerItemCommunityHandler) you used a selector, not this hook.

I'm curious about what other reviewers think about this pattern (cc. @tomek)

web/selectors/thread-selectors.js
111

This one is indeed really simple. Possible benefits of having simple selectors include:

  • giving them a name, which explains the purpose if that's not obvious
  • extracting simple, but repeated logic, e.g. nullability checks
  • simplifying the usage of repeated state (compare this to repeated usages of selecting current user id)
  • forcing explicit type annotation

The downside is that this type of selector, which is a hook, can't be efficiently composed. If instead only the state => data function was extracted, we could efficiently compose it in e.g. createSelector. But that usually doesn't matter at all.

So overall, I think it is ok to have simple selectors, but only when they can be named well. It's also ok to select by hand. If the selector is more complicated, extracting it makes more sense.

I still prefer selecting by hand, but this approach is also correct

web/selectors/thread-selectors.js
111

Thanks for the thorough response!

This revision is now accepted and ready to land.Mar 9 2023, 1:53 AM

I will think this through. Since I wrote this, some similar selectors were written (be me), and I would like to either unify them a bit or remove some of them. I will follow up next week