Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Media Player - Limit Unnecessary re-renders #3238

Merged
merged 8 commits into from
Mar 13, 2020
Merged

Media Player - Limit Unnecessary re-renders #3238

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0f93718
Prevent unnecessary re-renders
ryanmccombe Mar 10, 2020
173e23a
Simplify code
ryanmccombe Mar 10, 2020
bcb5cbe
Version bump
ryanmccombe Mar 11, 2020
1ea9c1c
Merge branch 'latest' into media-player-prevent-rerendering
ryanmccombe Mar 11, 2020
f7b2c61
Merge branch 'latest' into media-player-prevent-rerendering
ryanmccombe Mar 11, 2020
277113d
Add wrapping function
ryanmccombe Mar 11, 2020
bec71e4
Remove wrapping function
ryanmccombe Mar 12, 2020
e596c08
Merge branch 'latest' into media-player-prevent-rerendering
FK78 Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/psammead-media-player/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<!-- prettier-ignore -->
| Version | Description |
| ------------- | ---------------------------------------------------------------------------------------------------------------------------- |
| 2.7.5 | [PR#3238](https://github.com/bbc/psammead/pull/3238) Limit unnecessary re-rendering of canonical media player |
| 2.7.4 | [PR#3205](https://github.com/bbc/psammead/pull/3205) Removed isRequired from mediaInfo prop as not always required |
| 2.7.3 | [PR#3151](https://github.com/bbc/psammead/pull/3151) Talos - Bump Dependencies - @bbc/psammead-play-button |
| 2.7.2 | [PR#3113](https://github.com/bbc/psammead/pull/3113) Added bottom margin padding for live radio audio player |
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-media-player/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/components/psammead-media-player/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@bbc/psammead-media-player",
"version": "2.7.4",
"version": "2.7.5",
"description": "Provides a media player with optional placeholder",
"main": "dist/index.js",
"module": "esm/index.js",
Expand Down
17 changes: 13 additions & 4 deletions packages/components/psammead-media-player/src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { useState } from 'react';
import React, { useState, memo } from 'react';
import styled from 'styled-components';
import { string, bool, oneOf, shape } from 'prop-types';
import equals from 'ramda/src/equals';
import {
GEL_SPACING_DBL,
GEL_SPACING_QUAD,
Expand Down Expand Up @@ -30,7 +31,7 @@ const StyledAudioContainer = styled.div`
}
`;

export const CanonicalMediaPlayer = ({
const CanonicalMediaPlayerComponent = ({
showPlaceholder,
placeholderSrc,
placeholderSrcset,
Expand Down Expand Up @@ -76,6 +77,14 @@ export const CanonicalMediaPlayer = ({
);
};

// Component receives a "mediaInfo" object prop - this can cause unnecessary
// re-renders when the object reference changes, but the content is the same.
// We only rerender if the prevProps and nextProps fail deep equality check
export const CanonicalMediaPlayer = memo(
CanonicalMediaPlayerComponent,
(prevProps, nextProps) => equals(prevProps, nextProps),
);

Copy link
Contributor

@RichardPK RichardPK Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be unnecessarily explicit, as I'm describing essentially what ramda equals does implicitly, but, for future readability - I'm not sure it's clear that equals is being passed prevProps and nextProps?

I quite like the model of passing a function isEqual as the second argument in the memo and then declaring the function along these lines: (though I realise === may always evaluate to false even if the objects are principally 'the same', so may need rearranging)

const isEqual = (prevProps, nextProps) => {
  return prevProps === nextProps
}

I think this makes it clearer what's going on and maybe negates the need for adding comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers 👍 added wrapping function

we still need the deep equality check as === just checks if the objects are the same instance, ie, the same place in memory

the bug we're dealing with is parent components creating an entirely new object and passing it down - but that object has all the same values as the previous one

different objects will fail the === check but, if they have the same content, they'll pass the ramda equals check

Screen Shot 2020-03-11 at 17 49 21

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing because this wrapping function doesn't have test coverage 🤣

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-0.1% you naughty boy.

export const AmpMediaPlayer = ({
placeholderSrcset,
placeholderSrc,
Expand Down Expand Up @@ -107,7 +116,7 @@ export const AmpMediaPlayer = ({

export const MediaMessage = Message;

CanonicalMediaPlayer.propTypes = {
CanonicalMediaPlayerComponent.propTypes = {
placeholderSrc: string,
placeholderSrcset: string,
portrait: bool,
Expand All @@ -128,7 +137,7 @@ CanonicalMediaPlayer.propTypes = {
}),
};

CanonicalMediaPlayer.defaultProps = {
CanonicalMediaPlayerComponent.defaultProps = {
portrait: false,
showPlaceholder: true,
skin: 'classic',
Expand Down