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

is version 2 bigger bundle? #886

Closed
shaulgo opened this issue May 13, 2020 · 8 comments
Closed

is version 2 bigger bundle? #886

shaulgo opened this issue May 13, 2020 · 8 comments

Comments

@shaulgo
Copy link

shaulgo commented May 13, 2020

Hey, I thought version 2 came to improve bundle size by using the lazy loading of players. But bundlephobia seems to disagree. Is there more too it?
thanks :)

https://bundlephobia.com/result?p=react-player@2.0.1
image

@cookpete
Copy link
Owner

My guess is that BundlePhobia doesn't factor in lazy loading. It probably just sees the require(…) statements in the compiled players index file and pulls in all the files for all players.

@ovbm
Copy link

ovbm commented May 19, 2020

Screenshot 2020-05-19 at 09 24 07

That's how webpack works. I wanted to import only the FilePlayer, but it seems that option was removed in version 2. Here's the same app with import Fileplayer from 'react-player/lib/players/FilePlayer'; in 1.15.3:

Screenshot 2020-05-19 at 09 35 23

@cookpete
Copy link
Owner

Ok so it seems like I haven't quite implemented lazy players correctly. Babel compiles these lines:

https://github.com/CookPete/react-player/blob/32bb251a60f1a6449bd20e6dcf4c7dbfaeab02b7/src/players/index.js#L46-L50

into this:

{
  key: 'youtube',
  canPlay: function canPlay(url) {
    return _patterns.MATCH_URL_YOUTUBE.test(url);
  },
  lazyPlayer: (0, _react.lazy)(function () {
    return Promise.resolve().then(function () {
      return _interopRequireWildcard(require('./YouTube'));
    });
  })
}

I'm guessing for lazy loading to work it need to remain as import(), eg:

{
  key: 'youtube',
  canPlay: function canPlay(url) {
    return _patterns.MATCH_URL_YOUTUBE.test(url);
  },
  lazyPlayer: (0, _react.lazy)(function () {
    return import('./YouTube');
  })
}

The problem is that every player is then added as a chunk to the build, as there's no way of knowing at build time what players will be used. Maybe that's ok, but maybe I need to consider bringing back individual player imports…

@shaulgo
Copy link
Author

shaulgo commented May 20, 2020

As far as I know, multiple chunks is the way it's supposed to be.

@cookpete
Copy link
Owner

@shaulgo Try building with react-player@2.0.2-beta.0 and let me know if that helps. It's built with @babel/plugin-proposal-dynamic-import excluded from the babel env preset, so the built files now have import() for lazy loading.

@shaulgo
Copy link
Author

shaulgo commented May 21, 2020

amazing :) works. Checked it on our app and it split it into chunks and worked out of the box. It also looks very very nice in bundlephobia.
image

@cookpete
Copy link
Owner

Good to know! I'll work on getting this out. @shaulgo @ovbm Can you forsee any issues leaving import() statements in the compiled code?

Relevant comment: babel/babel#10273 (comment)

cookpete added a commit that referenced this issue Jun 7, 2020
Fixes #912
Fixes #907
Fixes #865
Sort of fixes #910
Sort of fixes #902
Affects #886
Affects #738
@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.

It's also worth noting that the BundlePhobia size will now go back up to ~17kb gzipped as the default import still includes logic for all players.

Webmaster1116 added a commit to Webmaster1116/video-player that referenced this issue May 20, 2021
Fixes cookpete/react-player#886
@babel/preset-env@7.5 added `@babel/plugin-proposal-dynamic-import`, which transformed the `import()` statements
More info: babel/babel#10273 (comment)
webmiraclepro added a commit to webmiraclepro/video-player that referenced this issue Sep 9, 2022
Fixes cookpete/react-player#886
@babel/preset-env@7.5 added `@babel/plugin-proposal-dynamic-import`, which transformed the `import()` statements
More info: babel/babel#10273 (comment)
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

3 participants