Details
The component itself is tested together with subsequent diffs where it is used.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/media/encrypted-image.react.js | ||
---|---|---|
33–39 | I am not necessarily opposed to setting state from the function body (I know this is an accepted pattern), but it's not clear to me if it's necessary here. Is there some reason this doesn't work? const prevConnectionStatusRef = React.useRef(connectionStatus); const attemptRef = React.useRef(0); if (prevConnectionStatusRef.current !== connectionStatus) { if (!source && connectionStatus === 'connected') { attemptRef.current++; } prevConnectionStatusRef.current = connectionStatus; } Won't this have the same effect, without requiring additional executions of the function body? | |
66–99 | This part seems copy-pasted from Multimedia. Is it possible to instead factor out the part of Multimedia you want? I think @ginsu also has a need for a component like Multimedia that takes a URI instead of a MediaInfo for his avatars work. Rather than have it copy-pasted in three places I think it would be good to factor out |
native/media/encrypted-image.react.js | ||
---|---|---|
66–99 | Sorry, I meant to say RemoteImage, not Multimedia |
native/media/encrypted-image.react.js | ||
---|---|---|
33–39 | This way is officially recommended. A very similar example in official React docs: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes |
native/media/encrypted-image.react.js | ||
---|---|---|
33–39 | I'm familiar with that example, but I don't think it's comparable. Let me know if you think I'm misunderstanding. I think the link you shared explains that it is better to set state in the function body than to have it done in a useEffect. In this case, neither setting state in the function body nor setting state in a useEffect is necessary. In fact, no setting of state should be necessary at all... The difference between state and refs is that state changes trigger rerenders. We already have a state change to trigger the rerender (connectionStatus), so we don't need any more state at all. |
- Replaced prevConnectionStatus state with ref. Kept attempt as state, because it needs to trigger hook and re-render when changed.
- Extracted common logic to LoadableImage component - separate diff D7232
Sorry for requesting changes again, but I have a couple of questions. If you don't think any changes are necessary, please feel free to respond to my questions and re-request review!
native/media/encrypted-image.react.js | ||
---|---|---|
29 ↗ | (On Diff #24329) |
Can you clarify further what you mean by this? My impression was that the rerender triggered by the connectionStatus change would update the attempt before rerendering, but I'm guessing you tested this and I'm missing something. I'm not necessarily opposed to setting state from the function body if it's really necessary, but I'd like to understand why. |
31–36 ↗ | (On Diff #24329) | Given that both RemoteImage and RemoteImage seem to need this functionality, I'm wondering why you didn't factor it out into LoadableImage |
66–99 | Yes, although that is a draft diff not meant for review |
Re-requesting review to verify my point of view
native/media/encrypted-image.react.js | ||
---|---|---|
29 ↗ | (On Diff #24329) |
Yes, but that attempt change isn't observed if it's a ref. Value change should trigger the effect to fire again. Ref is mutable and non-reactive so it cannot be an effect dependency. Correct me if you think there's a better solution for this. |
31–36 ↗ | (On Diff #24329) | Answered in https://phab.comm.dev/D7232#inline-47643 |
Have you tried the code snippet I suggested in my first review on Tuesday? Does it work or no?
native/media/encrypted-image.react.js | ||
---|---|---|
29 ↗ | (On Diff #24329) |
I think it's fine to have a ref's value as a dependency for an effect, eg. attemptRef.current. I don't understand what you mean by "change isn't observed"... I guess you're thinking of passing attemptRef instead of attemptRef.current? |
native/media/encrypted-image.react.js | ||
---|---|---|
29 ↗ | (On Diff #24329) |
I wish I agreed here, it would make things simple ;) However React ESLint explicitly says this: Also, I made a JSFiddle demonstrating the issue (look at the console at the bottom-right for console.logs) According to React docs, "effects run as a result of rendering." so a re-render is needed to trigger the effect. |
(Resigning to remove from my queue since it seems like @ashoat has got it, feel free to re-add me if there's anything specific)
Thanks for the detailed explanation and code example, @bartek – it looks like your approach is correct here