React wrapper for existing library

Question

https://github.com/BingXiong1995/react-flv-player/blob/master/lib/wrapper/ReactFlvPlayer.js

import React, { Component } from 'react';
import flvjs from './flv.min';
import PropTypes from 'prop-types';
class ReactFlvPlayer extends Component {
 constructor(props) {
 super(props);
 this.myRef = React.createRef();
 this.flvPlayerRef = element => {
 this.flvPlayerRef = element;
 };
 }
 componentDidMount() {
 const {type , url, isLive, enableStashBuffer, stashInitialSize, hasAudio, hasVideo, handleError, enableWarning, enableError} = this.props;
 // 组件挂载后,拿到Ref进行操作
 if (flvjs.isSupported()) {
 const flvPlayer = flvjs.createPlayer({
 type,
 isLive,
 url,
 hasAudio,
 hasVideo
 },{
 enableStashBuffer,
 stashInitialSize
 });
 flvjs.LoggingControl.enableError = false;
 flvjs.LoggingControl.enableWarn = enableWarning;
 flvPlayer.attachMediaElement(this.myRef.current); // 将这个DOM付给第三方库
 flvPlayer.load();
 flvPlayer.play();
 flvPlayer.on('error', (err)=>{
 // console.log(err);
 handleError(err);
 });
 }
 }
 render() {
 const { height, width, isMuted,showControls } = this.props;
 return (
 <div>
 <video
 controls={showControls}
 muted={{isMuted}}
 ref={this.myRef}
 style={{height, width}}
 />
 </div>
 );
 }
}
ReactFlvPlayer.propTypes = {
 type: PropTypes.string,
 url: PropTypes.string.isRequired,
 isLive: PropTypes.bool,
 showControls: PropTypes.bool,
 hasAudio: PropTypes.bool,
 hasVideo: PropTypes.bool,
 enableStashBuffer: PropTypes.bool,
 stashInitialSize: PropTypes.number,
 height: PropTypes.string,
 width: PropTypes.string,
 isMuted: PropTypes.bool,
 enableWarning: PropTypes.bool,
 enableError: PropTypes.bool,
 handleError: PropTypes.func
};
ReactFlvPlayer.defaultProps = {
 type: 'flv',
 isLive: true,
 hasAudio: true,
 hasVideo: true,
 showControls: true,
 enableStashBuffer: true,
 stashInitialSize: 128,
 height: '100%',
 width: '100%',
 isMuted: false,
 handleError: (err)=>{console.log(err)},
 enableWarning: false,
 enableError: false
};
export default ReactFlvPlayer;

Wrote some wrapper a long time ago. I am wondering if I could have done it in a better way. What are some of the improvements I could make or issues with the code. Thanks.

Answer 1

flvPlayerRef?

In the constructor, you have

this.myRef = React.createRef();
this.flvPlayerRef = element => {
 this.flvPlayerRef = element;
};

This is pretty confusing. The property is either a function or an element, depending on whether it was called as a function before, and either way, it's not a ref, so it's also misnamed. It also isn't used anywhere else in the code, and consumers of the instance already can get a reference to the <video> element through the myRef property.

I'd remove flvPlayerRef completely, and also rename the less-than-informative myRef property name to videoRef or to flvPlayerRef.

At that point, you can make things concise by using class fields instead of a constructor:

class ReactFlvPlayer extends Component {
 videoRef = React.createRef();
 componentDidMount() {
 // ...

You can also consider using a functional component instead of a class-based component, as React tentatively recommends for new code - but that's not required.

Destructured props

This line is hard to read:

const {type , url, isLive, enableStashBuffer, stashInitialSize, hasAudio, hasVideo, handleError, enableWarning, enableError} = this.props;

When there are more than 2 or 3 properties to be destructured, I'd recommend putting each on a separate line

const {
 type,
 url,
 isLive,
 // ...
} = this.props;

But, in this case, a significant fraction of the properties are used only to be passed into flvjs.createPlayer later. Consider using rest syntax to collect those options into a single object, without having to specify each one individually:

const {
 enableStashBuffer,
 stashInitialSize,
 handleError,
 enableWarning,
 enableError,
 ...createPlayerOptions
} = this.props;

The enableError variable is not used. If that's deliberate, best to not extract it from the props in the first place. Or maybe you meant to assign it to LoggingControl? Change

flvjs.LoggingControl.enableError = false;

to

flvjs.LoggingControl.enableError = enableError;

Nicer indentation Rather than creating another indentation block after checking if flvjs is supported, you can consider returning early if it's not supported:

componentDidMount() {
 if (!flvjs.isSupported()) {
 return;
 }
 const {
 enableStashBuffer,
 stashInitialSize,
 handleError,
 enableWarning,
 enableError,
 ...createPlayerOptions
 } = this.props;
 const flvPlayer = flvjs.createPlayer(
 createPlayerOptions,
 {
 enableStashBuffer,
 stashInitialSize
 }
 );
 // etc

Returning early is pretty nice, especially with more complex logic which would otherwise require multiple levels of indentation, which can get pretty hard to read.

Spacing There are a few places where I'd expect to see a space but don't see any given the code style in the rest of the script, or where I see spaces where there probably shouldn't be any, like const {type , url,, },{, (err)=>{, const { height, width, (do you want leading/trailing space when destructuring and with objects, or no?).

Whatever you want your code style to be, it would be good to be consistent - consider using ESLint to keep your style consistent, to fix things automatically, and warn you of potential bugs before they turn into runtime errors.