Would be nice if we could memoize this with React.useMemo() since it's dependent on the value of this.props.canReply, but unfortunately ComposedMessage is not a functional component.

If it can be done quickly, maybe we can convert it, otherwise probably not worth it.

Nice! These are no longer optional parameters.

This is a nit, but in some places in the codebase I think we don't add a newline between the props and the function header for components. Not sure what the best style is here so deferring to @atul @tomek or @jacek. I've seen both, but I think I've seen more non-newlines than components with newlines.

Why was this invariant removed?

Can also be memoized if RobotextMessage were functional.

Good call! should be pretty easy, I will work on that (and also on RobotextMessage)

For me the additional new line between props and component improve readability - we divide a large code block and it's more pleasing to the eye but I'll wait for other reviewers' opinion.

Since mouseOverMessagePosition is a required field (see line 54) there is no longer a need for invariant.

Ah right! Good catch.

I would recommend just quickly memoizing this inside ConnectedComposedMessage. Given that ComposedMessage has a lot of callbacks, and given that there is no way to define a callback in a functional component without requiring it to be recalculcated whenever a relevant prop changes, I would probably keep the ComposedMessage as a class component.

Ideally you should always look for and then try to follow existing conventions in a codebase. If you find a convention you don't like, then it's worth bringing up to the team to see if we want to change it. But the default should always be "find & follow existing convention" rather than "introduce personal convention"... otherwise there would be no conventions at all, since everybody has a different personal style.

@abosh is right in pointing out the issue here – { canReply: false } is recalculated on very render. However, I don't think his proposed solution of memoizing makes sense...

There's no point in memoizing something that is exactly the same every time. You can just define const cannotReply = { canReply: false } at the root scope, and pass in cannotReply here every time

Note that the benefits of doing this only apply if something is looking at whether messageState changes downstream. I can see that the definition of onReplyButtonClick will be recalculated when this changes (something that would never happen for a class component, by the way... see my comment above regarding ComposedMessage), but the benefits of memoizing props are most effective when the whole downstream component is wrapped with React.memo. We might want to do that for MessageTooltip, not sure.

@ashoat I totally agree with you, "find & follow existing convention" is the way it should go.

I only want to mention that I didn't mean to "introduce personal convention...", I did look for a convention but I saw files with the new line between props and component as well as without. That said I wasn't sure if any convention even exists and I chose the approach which I thought was better.

From your response and the fact that there are more files without space I suppose the newline should not be present here - so I will update this.
Sorry for the confusion.

That's totally fair... honestly we probably don't have much of a convention on this anymore... it's probably almost 50/50 nowadays 😅

@atul I also think it looks better, but because of how canReply is defined flow accepts it only if we pass value explicitly as true or false, not as boolean.

Ah yeah you’re totally right, thanks for explaining