Wrapper React para biblioteca existente

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;

Escreveu algum invólucro há muito tempo. Estou me perguntando se eu poderia ter feito de uma maneira melhor. Quais são algumas das melhorias que eu poderia fazer ou problemas com o código. Obrigado.

Answer 1

flvPlayerRef?

No construtor, você tem

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

Isso é muito confuso. A propriedade é uma função ou um elemento, dependendo se foi chamada como uma função antes e, de qualquer forma, não é um ref, portanto, também está nomeada incorretamente. Ele também não é usado em nenhum outro lugar do código, e os consumidores da instância já podem obter uma referência ao <video>elemento por meio da myRefpropriedade.

Gostaria de remover flvPlayerRefcompletamente e também renomear o nome da propriedade menos que informativa myRefpara videoRefou para flvPlayerRef.

Nesse ponto, você pode tornar as coisas concisas usando campos de classe em vez de um construtor:

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

  componentDidMount() {
    // ...

Você também pode considerar o uso de um componente funcional em vez de um componente baseado em classe, como o React recomenda provisoriamente para o novo código - mas isso não é obrigatório.

Adereços desestruturados

Esta linha é difícil de ler:

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

Quando houver mais de 2 ou 3 propriedades a serem desestruturadas, recomendo colocar cada uma em uma linha separada

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

Mas, neste caso, uma fração significativa das propriedades é usada apenas para ser passada flvjs.createPlayerposteriormente. Considere usar a sintaxe rest para coletar essas opções em um único objeto, sem precisar especificar cada uma individualmente:

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

A enableErrorvariável não é usada. Se isso for deliberado, é melhor não extraí-lo dos suportes em primeiro lugar. Ou talvez você quisesse atribuí-lo LoggingControl? mudança

flvjs.LoggingControl.enableError = false;

para

flvjs.LoggingControl.enableError = enableError;

Recuo mais agradável Em vez de criar outro bloco de recuo após verificar se flvjs é compatível, você pode considerar retornar mais cedo se não for compatível:

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

  const flvPlayer = flvjs.createPlayer(
    createPlayerOptions,
    {
      enableStashBuffer,
      stashInitialSize
    }
  );
  // etc

Voltar mais cedo é muito bom, especialmente com uma lógica mais complexa que, de outra forma, exigiria vários níveis de indentação, o que pode ser muito difícil de ler.

Espaçamento Existem alguns lugares onde eu esperaria ver um espaço, mas não vejo nenhum dado o estilo do código no resto do script, ou onde eu vejo espaços onde há provavelmente não deve ser qualquer, como const {type , url,, },{, (err)=>{, const { height, width,(você quer espaço inicial / final ao se desestruturar e com objetos, ou não?).

Seja qual for o seu estilo de código, seria bom ser consistente - considere usar o ESLint para manter seu estilo consistente, para consertar coisas automaticamente e avisar sobre possíveis bugs antes que se transformem em erros de tempo de execução.