itpresidents / thesis-archive-2020

10 stars 1 forks source link

Remove global AddMessage to message hub. #48

Closed oveddan closed 4 years ago

oveddan commented 4 years ago

https://github.com/oveddan/thesis-archive-2020/blob/6a811e5e2bcb3665e800d46b81bc51eb38d937fd/src/components/MessageHub.tsx#L117

The ability to add messages to the MessageHub is great. The main issue is that we are creating a global sort of event and pass data directly through it bypassing the component hierarchy. It's better to either have a state in a root component that gets updated, or to do it through context.

The way it's done now it can have bugs because you have to manually remember to unbind the event, or else it can get bound twice. You can see this result in some behavior that happens sometimes like the MessageHub appearing twice.

It would be better to do it the react way in which you explicitly define functions on components or the context that are called; this way the cleanup is taken care of for you.

oveddan commented 4 years ago

Also since there is only one message - we can just make it set a single message, like const [message, setMessage] = useState<string>(); in Header.tsx and just pass those to the children where needed.

EonYang commented 4 years ago

When I was making this I expected this message hub to be more useful than only show a specific message once. For instance, we can show a "Live stream will start at dd-hh-mm EST" on Videos. Or whatever messages we want to deliver to the users. If we want to achieve this goal, the messages state should be able to get set in any component. So we will either have it stored in the root component then pass it down to other components, which is not an issue now, since we only have one message. However I really want it to look like it's scalable. I don't want to introduce redux to our project, so I'll try to solve this with Context and useReducer. When I wrote this component I didn't know about reducer, now it's time to give it a try to see if it's capable for this task~

oveddan commented 4 years ago

ah I see - yeah it would be nice to set it anywhere. I think context would make sense. useReducer is more for when you want to figure out complex state.

EonYang commented 4 years ago

60

EonYang commented 4 years ago

Merged