Repository navigation
NXT-21175: Migrate VideoPlayer/Video.js to functional component. - #460
alexandrumorariu wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/functionalComponents #460 +/- ##
================================================================
+ Coverage 83.54% 83.85% +0.30%
================================================================
Files 158 158
Lines 7908 7954 +46
Branches 2446 2454 +8
================================================================
+ Hits 6607 6670 +63
+ Misses 1011 995 -16
+ Partials 290 289 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }, []); | ||
|
|
||
| // Equivalent of componentWillUnmount | ||
| useLayoutEffect(() => () => { |
There was a problem hiding this comment.
Drop this effect. setVideoRef already calls setMedia(null) when React detaches the ref, on unmount and when setMedia changes.
| // Keys from the previous render. They replace the instance fields the class mutated in | ||
| // `getKeys()`. State is only updated during render when the sources change. |
There was a problem hiding this comment.
This comment explains the code by comparing it with the old class
Could we describe what the state is for instead?
| // React calls the ref callback with `null` when the video is removed or unmounted, and when | ||
| // `setMedia` changes, so the previous `setMedia` is always cleared, as `componentDidUpdate` and | ||
| // `componentWillUnmount` did. |
There was a problem hiding this comment.
Same as above: “as componentDidUpdate and componentWillUnmount did” refers to code that no longer exists.
| preloadVideoRef.current = node; | ||
| }, []); | ||
|
|
||
| // Equivalent of componentDidUpdate. A layout effect keeps the class timing. |
There was a problem hiding this comment.
also describes the migration rather than the code. Suggest saying what the effect does and why it is a layout effect:
| const prevProps = prevPropsRef.current; | ||
| prevPropsRef.current = {source, preloadSource}; | ||
|
|
||
| // componentDidUpdate does not run on mount |
There was a problem hiding this comment.
// skip the initial mount; only react to changes.
| static defaultProps = { | ||
| mediaComponent: 'video' | ||
| }; | ||
| const {keys, prevPreloadKey: currentPreloadKey} = keysChanged ? nextKeyState : keyState; |
There was a problem hiding this comment.
keysChanged ? nextKeyState : keyState isn’t needed.
When setKeyState is called during render, React throws away this render’s output and runs the component again, so the nextKeyState branch never reaches the screen.
When nothing changed, nextKeyState already holds the same values as keyState, including the same keys array. keysChanged is only needed to decide whether to call setKeyState:
| const {keys, prevPreloadKey: currentPreloadKey} = keysChanged ? nextKeyState : keyState; | |
| if (keysChanged) { | |
| setKeyState(nextKeyState); | |
| } | |
| const {keys, prevPreloadKey: currentPreloadKey} = nextKeyState; |
| const videoRef = useRef(null); | ||
| const preloadVideoRef = useRef(null); | ||
| const preloadLoadStartRef = useRef(null); | ||
| const prevPropsRef = useRef(null); |
There was a problem hiding this comment.
prevPropsRef keeps the whole source / preloadSource nodes, so the effect calls getKeyFromSource 4 times on every render. VideoPlayer re-renders this component often during playback. The class did the same, so this isn’t a regression, but storing the previous keys would cut it to 2 calls and drop prevSource / prevPreloadSource:
check line 114-120
`const prevKeysRef = useRef(null);
// ...
useLayoutEffect(() => {
const key = getKeyFromSource(source);
const preloadKey = getKeyFromSource(preloadSource);
const prevKeys = prevKeysRef.current;
prevKeysRef.current = {key, preloadKey};
// skip the initial mount; only react to changes
if (!prevKeys) return;
const {key: prevKey, preloadKey: prevPreloadKey} = prevKeys;
// ...
});`
| const {keys, prevPreloadKey: currentPreloadKey} = keysChanged ? nextKeyState : keyState; | ||
|
|
||
| // if preload is unset, clear the key so we don't render that media node at all | ||
| const [sourceKey, preloadKey] = currentPreloadKey ? keys : keys.slice(0, 1); |
There was a problem hiding this comment.
There are now four different “preload key” variables.
getKeys has a local preloadKey (the source key), the render has preloadKey (the slot key, 'media-1' / 'media-2'), the effect has preloadKeyFromSource, and there is currentPreloadKey.
The preloadKeyFromSource rename exists only because the render’s preloadKey clashes with the effect’s.
Renaming the slot keys makes the difference clear and lets the effect keep its natural names:
const [sourceSlotKey, preloadSlotKey] = currentPreloadKey ? keys : keys.slice(0, 1); // ... {sourceSlotKey ? (<Media ... key={sourceSlotKey} ... />) : null} {preloadSlotKey ? (<Media ... key={preloadSlotKey} ... />) : null}
|
|
||
| // In the case that the previous source equalled the previous preload (causing the | ||
| // preload video node to not be created) and then the preload source was changed, we | ||
| // need to guard against accessing the preloadVideo node. |
There was a problem hiding this comment.
we need a test also for this scenario "Nothing tests what happens next, when preloadSource changes to a different value"
Checklist
Issue Resolved / Feature Added
Migrated
VideofromVideoPlayerto a functional component.Resolution
Additional Considerations
Links
NXT-21175
Comments