[FIX] Chat display fix

[mike-lvov]

Chat context was removed by the previous commit. Restored useFirestoreConnect in this commit to fetch chats

[0xdevalias]

@mike-lvov When you say 'was removed by the previous commit, which PR and/or commit are you referring to? Can you please link it here. (Also, for future, when possible it's a good idea to link to the PR/etc just to reduce the cognitive burden for future, and to make it easier to 'trace back' and see where things were introduced/etc.)

From the description, I'm guessing it refers to this chain of PR's (probably the most recent of them):

• Restore venueChats after removal of contexts #1036
• Remove contexts - attempt 2 #1007
• Fix chat after removing contexts #981
• Revert "Remove contexts (#956)" #980
• Remove contexts #956

[0xdevalias]

@mike-lvov A bunch of scattered review comments/suggestions/etc. I think some of them warrant changes before getting this out, but if you think they will take too long/disagree, and that we should merge this to fix prod first, then follow up with another PR to improve these aspects, I am open to that as well.

[0xdevalias]

IMO we shouldn't be calling this sort of thing 'inside' a hook like this, it feels like it couples it too tightly. My preference in the past has been to pass the venueId we want to 'connect' to as a prop. Which will make it easier to write nice isolated tests/etc as well.

Suggested change

	import { useVenueId } from "hooks/useVenueId";
	import { useSelector } from "hooks/useSelector";
	export const useVenueChatConnect = () => {
	const venueId = useVenueId();
	import { useSelector } from "hooks/useSelector";
	export const useVenueChatConnect = (venueId: string) => {

If you make this change though (maybe even if you don't) then we'll need to ensure that the storeAs venueChats always refers to the same venue. It might be better to make the storeAs include the venueId.. eg.

storeAs: `${venueId}Chats`,

(and if we do that, we also need to make sure to update the useVenueChats below to correctly reference this as well)

[0xdevalias]

Partially fixed in 354167a, but that implementation doesn't account for the venueId changing and how that will impact the storeAs name as highlighted in #1059 (comment)

[0xdevalias]

Backlogged the above in https://github.com/sparkletown/internal-sparkle-issues/issues/110

[0xdevalias]

[musing] I don't think there is any issue with us calling this multiple times within the app, as I believe the lib will realise it's the same thing, and not 're-subscribe' to it. I was pondering at one stage that we could build our own logic into our 'connect' hooks (eg. useVenueChatConnect) that somehow detected if it had already been subscribed, and if so, didn't try again. But if the library does that for us, even better!

[0xdevalias]

When the filter is done within the selector, it is memo'd and the component will only be updated if the end result (eg. after filtering) changes. With the change made here, it is going to re-render the component whenever any chat changes, and then re-run the filter each time.

It would be better if we could do this as part of the selector itself. I would probably refactor useVenueChats to take some options as props. In this case, I would probably call it something like includeDeleted or withDeleted, and default it to false. Then, based on this prop, we can do the basic selector, or the one that filters deleted.

To get the best memoisation, we want the selector function reference to remain the same, that is generally why we extract them to the selectors file. In this case it may make sense to do it more 'inline' within the hook though, so we could use useCallback to ensure our selector function reference only changes when the props it relies on changes. Something like the following (within the useVenueChats hook):

const venueChatsSelector = useCallback((state) => {
  const venueChats = state.firestore.ordered.venueChats ?? [];

  if (withDeleted) {
    return venueChats;
  } else {
    return venueChats?.filter((chat) => chat.deleted !== true)
  }
}, [withDeleted])

If you wanted to keep the selector logic in the selectors.ts file, then we could basically extract the logic of the function described above into that file, and then just call that function within the useCallback within useVenueChats (which is probably the better way, as then all our selector logic is kept 'isolated' in that one file)

selectors.ts:

const makeVenueChatsSelector = ({ withDeleted = false}) => (state) => {
  const venueChats = state.firestore.ordered.venueChats ?? [];

  if (withDeleted) {
    return venueChats;
  } else {
    return venueChats?.filter((chat) => chat.deleted !== true)
  }
}

useVenueChats.ts:

const venueChatsSelector = useCallback((state) => makeVenueChatsSelector({ withDeleted })(state), [withDeleted])

[0xdevalias]

And if the above isn't done (which is my preferred choice), at the very very least we should be wrapping filteredChats with useMemo so it's not re-filtering on every render.

[mike-lvov]

I think this approach is what I'm striving to do as well. Did useMemo for now. Though, I don't think there is any memorization happening as we don't have libs like reselect for that. As far as I understand, useSelect serves only as a pure function to retrieve data from redux store. I can be mistaken though.

[0xdevalias]

useSelector definitely memo’s in 2 different ways without reselect:

• https://react-redux.js.org/api/hooks

[0xdevalias]

Backlogged in https://github.com/sparkletown/internal-sparkle-issues/issues/111

[0xdevalias]

@mike-lvov i just saw that this was targeting master rather than staging.. and has been merged. I’m not sure if there is some context I’m missing, but we open PR’s against staging, and then that gets deployed to master, as opposed to merging directly against master.

[mike-lvov]

@0xdevalias Yes, the normal flow is creating PRs against staging, merge that and then merge and deploy to master. However, the thing I merged was a hotfix. We didn't have core functionality(chats) working on the production, which I think is a case for hotfixes.

[0xdevalias]

They had been broken for a while, and the time difference of a couple
Of minutes to follow our normal 'flow' wouldn't have made a meaningful difference, so we still should follow staging -> prod as normal. The only time a hotfix directly against master would maybe make sense (and even then I'm not so sure to be honest) is if the timing was so critical that the extra couple of minutes would make/break us, which ideally is never going to be the case. Even during our live patching of critical issues during recent events we followed the staging -> prod flow, and it was sufficient for our needs.

By merging directly against master, our staging and master branches are now out of sync, and we have to do extra work to port the changes from master back to staging.

In essence, it gains us very little, and causes extra work to diverge from our standard flow.

[0xdevalias]

Chatting to @mike-lvov just now, it seems that the core 'issue' that required the PR to be raised against master here is that there are changes currently in staging that we may not want to deploy to production yet (so staging is essentially 'blocked').

This is an issue we've run into in the past, and hopefully will be able to resolve sooner rather than later, potentially by implementing 'per feature branch' environments so that we can clickthrough/etc before deploying to staging, and thus proactively know if it would break anything.