search

shiyiya / oplayer

:zap: Another HTM5 video player.
https://oplayer.vercel.app
MIT License
147 stars 19 forks source link

Remix react breaks with 1.0.27, works with 1.0.26 #16

Closed phpb-com closed 2 years ago

phpb-com commented 2 years ago

Hi again,

I ran into issues with my Remix React app after the upgrade to 1.0.27. This error pos-up once I click through to the video and then try to go back to the home page clicking the navigation. I did not look too deeply yet, but though it would help if I report it first. You may have an idea why.

TypeError: Cannot read properties of undefined (reading 'catch')
    at I.<anonymous> (http://127.0.0.1:8788/build/routes/video/$videoId-ZM2HA7D3.js:5077:41)
    at I.destroy (http://127.0.0.1:8788/build/routes/video/$videoId-ZM2HA7D3.js:4086:32)
    at http://127.0.0.1:8788/build/routes/video/$videoId-ZM2HA7D3.js:5604:52
    at safelyCallDestroy (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:16274:13)
    at commitHookEffectListUnmount (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:16401:19)
    at commitPassiveUnmountInsideDeletedTreeOnFiber (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17812:17)
    at commitPassiveUnmountEffectsInsideOfDeletedTree_begin (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17771:13)
    at commitPassiveUnmountEffects_begin (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17707:19)
    at commitPassiveUnmountEffects (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17695:11)
    at flushPassiveEffectsImpl (http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:18987:11)
phpb-com commented 2 years ago

This error only pops-up on the client side, and I am using ReactJS 18.2.0

phpb-com commented 2 years ago

Ah, thanks, I will wait for the next npm release before bumping the version then.

shiyiya commented 2 years ago

https://github.com/shiyiya/oplayer/commit/00e66cd0c0a45928653030ee23eb0ff942ac3499#diff-6593ce7b068dd4ab9fdd9e4e2260e1ab9fedc7af44251167f3a934729122fb0dL73

Note that some APIs have changed.

phpb-com commented 2 years ago

Thanks, I have been tracking your changes closely, I hope I did not miss anything else. ;-)

shiyiya commented 2 years ago

v1.0.28 released.

phpb-com commented 2 years ago

Tested and it works as expected, thanks and you are the best! 💯

phpb-com commented 2 years ago

Ok, sorry, but I have to take the part about "all works" back. I just did the test on the iPad and iPhone, and get the following error:

h4@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:4562:61
Fe2@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:4567:61
_e@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:4856:22
Tt@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:5540:5
@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:3995:29
forEach@[native code]
@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:3994:25
@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:3934:77
@http://127.0.0.1:8788/build/routes/video/$videoId-EW6Q7TAW.js:5607:53
commitAttachRef@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:16813:29
commitLayoutEffectOnFiber@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:16696:32
commitLayoutMountEffects_complete@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17506:42
commitLayoutEffects_begin@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17495:48
commitLayoutEffects@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:17447:36
commitRootImpl@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:18851:32
commitRoot@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:18775:27
finishConcurrentRender@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:18304:25
performConcurrentWorkOnRoot@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:18218:37
performConcurrentWorkOnRoot@[native code]
workLoop@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:200:50
flushWork@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:179:30
performWorkUntilDeadline@http://127.0.0.1:8788/build/entry.client-B7WEPQOE.js:387:50

When deployed in "prod", I get:

g@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:180:72192
bl@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:180:3403
Ol@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:208:78415
oc@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:310:96000
@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:18:49635
forEach@[native code]
@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:18:49614
@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:6:47849
@https://example.com/build/routes/video/$videoId-BYQJ4PNE.js:310:97691
Mi@https://example.com/build/entry.client-PRWD2PB5.js:8:101051
Qo@https://example.com/build/entry.client-PRWD2PB5.js:8:109172
Ua@https://example.com/build/entry.client-PRWD2PB5.js:8:108071
Qf@https://example.com/build/entry.client-PRWD2PB5.js:8:107617
Gf@https://example.com/build/entry.client-PRWD2PB5.js:8:117470
hn@https://example.com/build/entry.client-PRWD2PB5.js:8:116822
Va@https://example.com/build/entry.client-PRWD2PB5.js:8:112320
Va@[native code]
El@https://example.com/build/entry.client-PRWD2PB5.js:1:1742
yl@https://example.com/build/entry.client-PRWD2PB5.js:1:2133

(URL is replaced with example.com, sorry but it is still WiP.

phpb-com commented 2 years ago

This error happens when I go to the page with the player in it. Work on a desktop (Chrome) though,

shiyiya commented 2 years ago

What about https://oplayer.vercel.app ？

shiyiya commented 2 years ago

I cannot get valid information from the error

phpb-com commented 2 years ago

https://oplayer.vercel.app/ - work, but the difference is Remix app here. Something to do with the native code leaking into client side, I would speculate. I'll try to see if removing some of the code in-between the commits will help and narrow down on the issue (if I can).

shiyiya commented 2 years ago

Can you provide the code to call oplayer?

phpb-com commented 2 years ago

That I can, and I think I kinda know what might be the problem:

import ui from '@oplayer/ui'
import hls from '@oplayer/hls'
import ReactPlayer from '@oplayer/react'
import type { ReactOPlayerProps } from '@oplayer/react'
import type { HlsConfig } from 'hls.js'
import {
  isEmbedded,
  isIOS,
  isIOS13,
  isIPad13,
  isIPhone13,
  isIPod13,
  isMobile,
  isMobileSafari,
  isSmartTV,
  isWearable,
} from 'react-device-detect'

export function OPlayer(props: ReactOPlayerProps) {
  const hlsConfig: Partial<HlsConfig> = {
    debug: false,
    capLevelOnFPSDrop: true,
    capLevelToPlayerSize: true,
    manifestLoadingTimeOut: 10000,
    manifestLoadingMaxRetry: 4,
    manifestLoadingRetryDelay: 1000,
    manifestLoadingMaxRetryTimeout: 64000,
    levelLoadingTimeOut: 10000,
    levelLoadingMaxRetry: 4,
    levelLoadingRetryDelay: 1000,
    levelLoadingMaxRetryTimeout: 64000,
    fragLoadingTimeOut: 20000,
    fragLoadingMaxRetry: 6,
    fragLoadingRetryDelay: 1000,
    fragLoadingMaxRetryTimeout: 64000,
    testBandwidth: true,
    maxAudioFramesDrift: 1,
    // Buffer configuration
    maxBufferLength: 10,
    maxMaxBufferLength: 20,
  }

  const plugins = [
    hls({ hlsConfig: hlsConfig }),
    ui({
      theme: { primaryColor: '#BC0011' },
      airplay: true,
      pictureInPicture: true,
      miniProgressBar: true,
      autoFocus: true,
      screenshot:
        isIOS ||
        isIOS13 ||
        isIPhone13 ||
        isIPod13 ||
        isIPad13 ||
        isMobileSafari ||
        isSmartTV ||
        isWearable ||
        isEmbedded ||
        isMobile
          ? false
          : true,
      speed: ['2', '1.5', '1', '0.5'],
      fullscreen: true,
      fullscreenWeb:
        isIOS || isIOS13 || isMobileSafari || isSmartTV || isWearable
          ? false
          : true,
      hotkey: true,
    }),
  ]

  return (
    <ReactPlayer
      plugins={plugins}
      {...props}
    />
  )
}
phpb-com commented 2 years ago

Ok, the problem is here:

      fullscreenWeb:
        isIOS || isIOS13 || isMobileSafari || isSmartTV || isWearable
          ? false
          : true,

When set to true, all works as expected.

shiyiya commented 2 years ago

Is this an oplayer error?

phpb-com commented 2 years ago

I think so, something to do with disablement of the fullscreenWeb. I had similar issue in the past but you fixed it with some other changes, so I never bothered you with that.

phpb-com commented 2 years ago

To be clear if I go from 

      speed: ['2', '1.5', '1', '0.5'],
      fullscreen: true,
      fullscreenWeb:
        isIOS || isIOS13 || isMobileSafari || isSmartTV || isWearable
          ? false
          : true,
      hotkey: true,

to 

      speed: ['2', '1.5', '1', '0.5'],
      fullscreen: true,
      fullscreenWeb: true,
      hotkey: true,

everything starts working again.

shiyiya commented 2 years ago

Wait, I found the reason for the error.

phpb-com commented 2 years ago

Setting it to false on the desctop results in:

TypeError: Cannot read properties of null (reading 'removeAttribute')
    at h4 (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:4562:62)
    at Fe2 (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:4567:59)
    at _e (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:4856:19)
    at Tt (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:5540:3)
    at Object.apply (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:5555:18)
    at http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:3995:24
    at Set.forEach (<anonymous>)
    at I.<anonymous> (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:3994:18)
    at I.create (http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:3934:73)
    at http://127.0.0.1:8788/build/routes/video/$videoId-UH56RKTP.js:5607:47
phpb-com commented 2 years ago

Ah, we posted at the same time. Yes, I guess this error message is a better one.

shiyiya commented 2 years ago

A new version has been released, try it out and thanks for the feedback!

phpb-com commented 2 years ago

1.0.29 works with false and true for fullscreenWeb. Thanks for being awesome and fixing it!

shiyiya commented 2 years ago 
import ui from '@oplayer/ui'
//....

/* Put it on top or wrap it in moemo */
const hlsConfig: Partial<HlsConfig> = { }
const plugins = [ ]
/* *   **   *  */

export function OPlayer(props: ReactOPlayerProps) {

  return (
    <ReactPlayer
      plugins={plugins}
      {...props}
    />
  )
}
phpb-com commented 2 years ago

Yes, that is a good point. I moved it in later on to potentially pass props to the element. Never got to it yet :-(

shiyiya commented 2 years ago

Currently they(plugins) are not responsive and the update will do nothing

phpb-com commented 2 years ago

True, I had to find it out the hard way ;-) Thanks for spotting the bug in my code that caused some headaches already