React wrapper dla istniejącej biblioteki

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;

Napisałem jakieś opakowanie dawno temu. Zastanawiam się, czy mogłem to zrobić w lepszy sposób. Jakie ulepszenia mogę wprowadzić lub jakie problemy z kodem. Dzięki.

Answer 1

flvPlayerRef?

W konstruktorze masz

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

To jest dość zagmatwane. Właściwość jest albo funkcją, albo elementem, w zależności od tego, czy wcześniej była wywoływana jako funkcja, i tak czy inaczej, nie jest referencją, więc jest również błędnie nazwana. Nie jest również używany nigdzie indziej w kodzie, a konsumenci instancji mogą już uzyskać odwołanie do <video>elementu za pośrednictwem myRefwłaściwości.

Usunąłbym flvPlayerRefcałkowicie, a także zmieniłbym nazwę właściwości zawierającej mniej informacji myRefna videoReflub na flvPlayerRef.

W tym momencie możesz uczynić rzeczy zwięzłymi, używając pól klas zamiast konstruktora:

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

  componentDidMount() {
    // ...

Możesz także rozważyć użycie komponentu funkcjonalnego zamiast komponentu opartego na klasach, ponieważ React wstępnie zaleca dla nowego kodu - ale nie jest to wymagane.

Zniszczone rekwizyty

Ta linia jest trudna do odczytania:

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

Gdy do zniszczenia jest więcej niż 2 lub 3 właściwości, radzę umieścić każdą w osobnym wierszu

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

Ale w tym przypadku znaczna część właściwości jest wykorzystywana tylko do flvjs.createPlayerpóźniejszego przekazania . Rozważ użycie składni reszty, aby zebrać te opcje w jednym obiekcie, bez konieczności określania każdej z nich osobno:

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

enableErrorZmienna nie jest używany. Jeśli jest to zamierzone, najlepiej w ogóle nie wyciągać tego z rekwizytów. A może miałeś to przypisać LoggingControl? Zmiana

flvjs.LoggingControl.enableError = false;

do

flvjs.LoggingControl.enableError = enableError;

Ładniejszy wcięcie Zamiast tworzyć kolejny blok wcięcia po sprawdzeniu czy flvjs jest obsługiwany, można rozważyć powrót wcześnie, jeśli to nie obsługiwane:

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

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

Wczesne powracanie jest całkiem przyjemne, zwłaszcza przy bardziej złożonej logice, która w innym przypadku wymagałaby wielu poziomów wcięć, co może być dość trudne do odczytania.

Rozstaw Istnieje kilka miejsc, gdzie będę oczekiwać, aby zobaczyć miejsca, ale nie widać żadnych biorąc pod uwagę styl kodu w pozostałej części skryptu, lub gdy widzę obowiązuje tam, gdzie prawdopodobnie nie powinno być, jak const {type , url,, },{, (err)=>{, const { height, width,(czy chcesz spację wiodącą / końcową podczas destrukturyzacji i obiektów, czy nie?).

Niezależnie od tego, jaki ma być styl kodu, dobrze byłoby zachować spójność - rozważ użycie ESLint, aby zachować spójność stylu, automatycznie naprawiać rzeczy i ostrzegać o potencjalnych błędach, zanim zamienią się w błędy w czasie wykonywania.