Point is to use safer type declarations instead of invariants in Message Tooltip. More context here: phabricator link.
To solve this new type is needed: MessageStateType which will contain information that either user can not reply, or can and all needed props are provided.
Details
Checked flow types.
Tested text, media, and robotext messages to check if changes didn't affect current logic.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- web/remove-invariant
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
web/chat/composed-message.react.js | ||
---|---|---|
131–137 | 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. | |
web/chat/message-tooltip.react.js | ||
40–46 | Nice! These are no longer optional parameters. | |
56 | 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. | |
151–154 | Why was this invariant removed? | |
web/chat/robotext-message.react.js | ||
74 | Can also be memoized if RobotextMessage were functional. |
Thanks for taking a look @abosh!
web/chat/composed-message.react.js | ||
---|---|---|
131–137 | Good call! should be pretty easy, I will work on that (and also on RobotextMessage) | |
web/chat/message-tooltip.react.js | ||
56 | 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. | |
151–154 | Since mouseOverMessagePosition is a required field (see line 54) there is no longer a need for invariant. |
web/chat/message-tooltip.react.js | ||
---|---|---|
151–154 | Ah right! Good catch. |
web/chat/composed-message.react.js | ||
---|---|---|
131–137 | 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. | |
web/chat/message-tooltip.react.js | ||
56 | 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. | |
web/chat/robotext-message.react.js | ||
74 | @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. |
web/chat/message-tooltip.react.js | ||
---|---|---|
56 | @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. |
web/chat/message-tooltip.react.js | ||
---|---|---|
56 | That's totally fair... honestly we probably don't have much of a convention on this anymore... it's probably almost 50/50 nowadays 😅 |
Improve code, use memoization where needed (memoize MessageReplyProps inside ConnectedComposedMessage)
Looks great! Just two comments inline.
web/chat/composed-message.react.js | ||
---|---|---|
125 ↗ | (On Diff #15911) | What do you think about using this.props.messageReplyProps.canReply here? By doing that we would reduce the number of props this component uses. |
web/chat/message-tooltip.react.js | ||
128 ↗ | (On Diff #15911) | It is a good practice to make dependency list as specific as possible. In this case we're using messageReplyProps.canReply and messageReplyProps.setMouseOverMessagePosition so maybe we can depend just on these? I guess it might be a little more complicated because setMouseOverMessagePosition doesn't appear in all the variants of MessageReplyProps, but still it's worth trying to avoid the recomputation every time inputState changes. |
Instead of introducing a nested MessageReplyProps object to the MessageTooltipProps type, couldn't we modify MessageTooltipProps to handle both the canReply: false and canReply: true cases? Something like:
type BaseMessageTooltipProps = { +threadInfo: ThreadInfo, +item: ChatMessageInfoItem, +availableTooltipPositions: $ReadOnlyArray<TooltipPosition>, +mouseOverMessagePosition: OnMessagePositionWithContainerInfo, }; type MessageTooltipProps = | { +canReply: false, ...BaseMessageTooltipProps, } | { +canReply: true, ...BaseMessageTooltipProps +inputState: ?InputState, +setMouseOverMessagePosition: ( messagePositionInfo: MessagePositionInfo, ) => void, };
Here are the flow docs on "Disjoint Object Unions": https://flow.org/en/docs/types/unions/#toc-disjoint-object-unions
I think it would be cleaner and we'd be able to avoid nesting... feel free to re-request review if I'm missing something here
web/chat/robotext-message.react.js | ||
---|---|---|
32 ↗ | (On Diff #16004) | Thoughts on naming this something like messageReplyProps instead of trying to describe the contents of the object? |
web/chat/composed-message.react.js | ||
---|---|---|
131–139 ↗ | (On Diff #16045) | We have a couple of rules https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e regarding the ternary, and in this case (due to 3rd point) it should be avoided. The solution with ternary doesn't look that bad in this case, so if there are Flow issues with the proposed approach, I don't think it is worth spending too much time on it. |
web/chat/message-tooltip.react.js | ||
50–57 ↗ | (On Diff #16045) | A nit: it is safer to destructure first and then assign the props - by doing that we avoid an issue where something destructured would override our props. |
Thanks for flattening MessageTooltipProps! Looks a lot cleaner imo
web/chat/composed-message.react.js | ||
---|---|---|
131–151 ↗ | (On Diff #16046) | Could this be further flattened to something like this: let messageTooltipProps = { canReply: this.props.canReply, threadInfo, item, availableTooltipPositions, mouseOverMessagePosition: this.props.mouseOverMessagePosition, }; if (this.props.canReply) { messageTooltipProps = { ...messageTooltipProps, setMouseOverMessagePosition: this.props.setMouseOverMessagePosition, inputState: this.props.inputState, }; } messageTooltip = <MessageTooltip {...messageTooltipProps} />; Feel free to land as-is or go with whatever you prefer |
web/chat/composed-message.react.js | ||
---|---|---|
131–151 ↗ | (On Diff #16046) | Ah yeah you’re totally right, thanks for explaining |