Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: lazy players #738

Closed
trurl-master opened this issue Nov 5, 2019 · 21 comments
Closed

Feature request: lazy players #738

trurl-master opened this issue Nov 5, 2019 · 21 comments

Comments

@trurl-master
Copy link

trurl-master commented Nov 5, 2019

Motivation: On my page I don't know what player will be used in each particular case and many of the players will never be used, so I would like to lazily load only one player in each particular case.

Something like this:

import React, { Suspense, lazy } from 'react'
import { whichPlayerCanPlay } from 'react-player'

const players = {
  youtube: () => import('react-player/lib/players/YouTube'),
  vimeo: () => import('react-player/lib/players/Vimeo'),
  file: () => import('react-player/lib/players/FilePlayer')
}

const MyPlayer = (props) => {
  const { url } = props

  const playerName = whichPlayerCanPlay(url, { players: ['youtube', 'vimeo', 'file'] })

  if (!playerName) {
    return null
  }

  const LazyPlayer = lazy(players[playerName]);

  return (
    <Suspense fallback={<div>Loading...</div>}>
      <LazyPlayer {...props} />
    </Suspense>
  )
}

The problem here is that the canPlay logic is inside of each individual player file and to know whether a player is capable of playing the url I need to download it. Could the canPlay logic be extracted from each individual player to a separate file?

@cookpete
Copy link
Owner

cookpete commented Nov 6, 2019

Hey @trurl-master, yeah this has been on my radar for a long time. It would reduce the bundle size massively and essentially render the single player imports redundant. I can look at implementing this, but don't have an exact timeline.

@cookpete
Copy link
Owner

I've pushed my first attempt at this as an alpha version of 2.0.0 (as there are a few breaking changes). Try it out with:

npm install react-player@2.0.0-alpha.0

@ramlez
Copy link

ramlez commented Feb 4, 2020

(as there are a few breaking changes)

Hi, @cookpete could you please let know what braking changes are in 2.0 version? I've successfully added the alpha version to my project and haven't spotted any breaking changes or problems yet.

@cookpete
Copy link
Owner

@ramlez Namely the single player functionality eg

import YouTubePlayer from 'react-player/lib/players/YouTube'

becomes pretty much redundant, as only the Youtube player will be loaded if you only ever use a Youtube URL.

I should be able to get a bit closer to releasing this in the next couple of days.

@cookpete
Copy link
Owner

I'm also considering removing the preload functionality and leaving it as a legacy feature in older versions – autoplay policies are making it redundant, and it's a fairly hacky feature anyway.

@dbismut
Copy link

dbismut commented Apr 21, 2020

Hey @cookpete @trurl-master, until Suspense and lazy support SSR rendering, maybe you could consider using Loadable Components instead?

@cookpete
Copy link
Owner

cookpete commented Apr 25, 2020

v2.0.0 is now here, which includes lazy loading players using lazy and Suspense. @dbismut unfortunately I had written all the lazy logic before your comment.

@dbismut Is there much benefit to including the lazy player code in SSR? Most of the time the only thing rendered is a single div or iframe, which is populated on the client when the third party player API loads.

@dbismut
Copy link

dbismut commented Apr 25, 2020

@cookpete it's not so much that SSR actually makes sense in this case, but rather than using Suspense and lazy will break SSR. In other words, if you have a look at this sandbox using Gatsby, and if you try to build it, you will have the following error.

image

... which redirects to this page: https://reactjs.org/docs/error-decoder.html/?invariant=294

Let me know if you prefer that I open a separate issue for this, as someone will open it sooner or later.

@tubbo
Copy link

tubbo commented Apr 26, 2020

^ i also had the above issue, wondering what to do about it...

@cookpete
Copy link
Owner

Is there a quick fix for Suspense to just not render anything during SSR, rather than breaking things? This seems like a stupid problem to have when ReactPlayer doesn’t really benefit from rendering anything on the server.

@dbismut
Copy link

dbismut commented Apr 26, 2020

Well, I guess you could detect if you're server-side:

const isBrowser = (typeof window !== 'undefined' && window.document)
return <div>{ isBrowser && <Suspense /> }</div>

@ericnation
Copy link

It's also breaking my Gatsby build with the same exact error posted by @dbismut. Wondering exactly how to get around this?

@tubbo
Copy link

tubbo commented Apr 27, 2020

@ericnation I downgraded to 1.15.3...that fixed the build and got me past this issue, but I don't really need any of these lazy/suspense features, so YMMV.

@winstonma
Copy link

winstonma commented Apr 27, 2020

@ericnation I guess you can use the method mentioned by @dbismut to get rid of the build-time problem.

   const isBrowser = (typeof window !== 'undefined' && window.document)
   ...
   <>
     { isBrowser && <ReactPlayer></ReactPlayer>}
  </>

Just wonder if (typeof window !== 'undefined' && window.document) would be included in react-player?

@dbismut
Copy link

dbismut commented Apr 28, 2020

Just wonder if (typeof window !== 'undefined' && window.document) would be included in react-player?

That’s the idea indeed!

@cookpete cookpete reopened this Apr 28, 2020
@cookpete
Copy link
Owner

This should be fixed in v2.0.1

@shortcircuit3
Copy link

Can you add this to the docs? Im trying to figure out how to improve load time.

@cookpete
Copy link
Owner

cookpete commented Jun 7, 2020

Note that from v2.2.0, the lazy loading version of ReactPlayer has moved to react-player/lazy. See MIGRATING.md for more info.

I'm not considering it a breaking change as things won't actually break – the only impact will be a slightly bigger bundle size until you start importing from react-player/lazy.

I've added clarification to the readme to only use it if your build pipeline supports dynamic import() statements.

@alex-major-digital
Copy link

alex-major-digital commented Feb 10, 2023

@cookpete it's not so much that SSR actually makes sense in this case, but rather than using Suspense and lazy will break SSR. In other words, if you have a look at this sandbox using Gatsby, and if you try to build it, you will have the following error.

image

... which redirects to this page: https://reactjs.org/docs/error-decoder.html/?invariant=294

Let me know if you prefer that I open a separate issue for this, as someone will open it sooner or later.

I'm getting this error too on build - I'm on version 2.11.2.

Any ideas?

@cookpete
Copy link
Owner

@alex-major-digital Are you importing react-player or react-player/lazy? The check for IS_BROWSER in 54f48c0 isn’t perfect so I would guess IS_BROWSER is true for whatever platform you are using, so it’s trying to render Suspense which is not supported.

It’s been over three years now since I added the lazy stuff. Clearly it is not written in the best way. If you can figure out why IS_BROWSER is true, a PR with a fix would be very welcome.

@alex-major-digital
Copy link

alex-major-digital commented Feb 10, 2023

@alex-major-digital Are you importing react-player or react-player/lazy? The check for IS_BROWSER in 54f48c0 isn’t perfect so I would guess IS_BROWSER is true for whatever platform you are using, so it’s trying to render Suspense which is not supported.

It’s been over three years now since I added the lazy stuff. Clearly it is not written in the best way. If you can figure out why IS_BROWSER is true, a PR with a fix would be very welcome.

Hi @cookpete, thanks for the quick reply! I get the error when importing both react-player & react-player/lazy. My site uses SSG, so IS_BROWSER is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants