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

WIP: Webpackify #5033

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
3 participants
@gkatsev
Copy link
Member

gkatsev commented Mar 20, 2018

No description provided.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Mar 20, 2018

initial assets output for the new webpack build:

┌──────────────┬───────────┬───────────┐
│ filename     │      size │   gzipped │
├──────────────┼───────────┼───────────┤
│ video.cjs.js │ 148.87 KB │  40.75 KB │
├──────────────┼───────────┼───────────┤
│ video.js     │  698.2 KB │ 159.45 KB │
├──────────────┼───────────┼───────────┤
│ video.min.js │ 172.26 KB │  48.47 KB │
└──────────────┴───────────┴───────────┘

compared to current master:

┌─────────────────────────────────────────────┬───────────┬───────────┐
│ filename                                    │      size │   gzipped │
├─────────────────────────────────────────────┼───────────┼───────────┤
│ video.cjs.js                                │ 630.82 KB │ 139.89 KB │
├─────────────────────────────────────────────┼───────────┼───────────┤
│ video.es.js                                 │ 630.58 KB │ 139.84 KB │
├─────────────────────────────────────────────┼───────────┼───────────┤
│ video.js                                    │ 695.03 KB │ 157.63 KB │
├─────────────────────────────────────────────┼───────────┼───────────┤
│ video.min.js                                │  188.8 KB │  49.79 KB │
└─────────────────────────────────────────────┴───────────┴───────────┘

Things of note:

  • can't output a video.es.js via webpack because it doesn't have a modules target, only umd, amd, and commonjs.
  • the cjs file is minimized and optimized by default thus the much smaller sizes
  • the video.js file, which is unminified, is 2K larger gzipped with webpack than with rollup
  • the video.min.js file is ~1K smaller than with rollup.
@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Mar 21, 2018

Though, I remembered that we haven't ran babel on the output yet, so, it might get a bit larger because of that. Hopefully, it'll stay about the same after compilation.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Mar 21, 2018

After adding in babel-loader, the video.min.js gzipped filesize is 59.88, which is a 20.27% increase in size.

@BrandonOCasey

This comment has been minimized.

Copy link
Contributor

BrandonOCasey commented Mar 21, 2018

You can also include external for the global dependency. This is how I did it in the other pr:

externals: {
  'global/window': 'window',
  'global': 'window',
  'global/document': 'document'
},

@BrandonOCasey BrandonOCasey added this to In Progress in 7.0 Mar 21, 2018

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Mar 22, 2018

We may end up not going forward with this due to the size increase from switching back to a bundler like webpack. After some fiddling locally, I got it to only be an increase of about 11% and with the IE8 removal work, it may be only about 10% increase but would be nice if we can avoid that increase.

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Mar 29, 2018

So, given that VHS is switching to rollup, we're going to stick with rollup for now as well. Superseded by #5057.

@gkatsev gkatsev closed this Mar 29, 2018

@gkatsev gkatsev deleted the webpackify branch Mar 29, 2018

@gkatsev gkatsev moved this from In Progress to Done in 7.0 Apr 2, 2018

@axten

This comment has been minimized.

Copy link
Contributor

axten commented Apr 4, 2018

I'm sad to see your decision..
I think the biggest problem using webpack is the plugin structure.
they register itself with a global available videojs and a webpack config hack is necessary:

new webpack.ProvidePlugin({
    'window.videojs': 'video.js',
}),

in my opinion, this should be fixed in es6 context in the plugins.

maybe like this api:

import videojs from 'video.js/dist/video.cjs';
import httpStreaming from '@videojs/http-streaming/dist/videojs-http-streaming';

import qualitySelector from 'silvermine-videojs-quality-selector';
import qualityLevels from 'videojs-contrib-quality-levels';

class Player {
    constructor() {
        
        videojs.addPlugins([
            httpStreaming,
            qualitySelector,
            qualityLevels
        ]);

        const player = videojs('player_id');
    }
}

do you think this is a good idea?
@gkatsev

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Apr 4, 2018

So, the main reason why we aren't going to use webpack is because switching to it from rollup altogether makes the file size too large. We were looking about rolling up our files and then running webpack over that but we ran into another issue with trying to inline VHS. VHS depends on Video.js currently and either we get a circular dependency where VHS gets an empty object as Video.js or Video.js gets included twice in the final build of a user's bundle.
One of the things we're looking into is splitting up Video.js in multiple subpackages so all the utilities would be in something like @videojs/core and/or @videojs/utils that VHS can require instead and then the video.js package would include all the different pieces subpackages and VHS.

Updating plugins to not auto register themselves isn't necessarily an issue for webpack vs rollup. And for VHS itself, registering it correctly is a bit tricky, compared to just a regular plugin.

currently, whether video.js will be packaged by rollup or webpack, a user should be able to do:

import videojs from 'video.js';
import '@videojs/http-streaming'; // this isn't even necessary once we ship 7.0 by default
import 'silvermine-videojs-quality-selector';

However, having the plugin export itself for the user to register isn't a bad idea. The main reason we do it currently so that we can easily export the plugin for <script> users and not require them do any work.

import videojs from 'video.js';
import qualitySelector from 'silvermine-videojs-quality-selector';

videojs.registerPlugin('qualityselector', qualitySelector);

is fine.

@axten

This comment has been minimized.

Copy link
Contributor

axten commented Apr 4, 2018

ok I agree that my problem is not related to webpack vs. rollup...
I tested your example and it worked without ProvidePlugin so I found out that I was importing the wrong file:
import '@videojs/http-streaming';
instead of
import '@videojs/http-streaming/dist/videojs-http-streaming';
fixed the error for me...
sorry for wasting your time !

@gkatsev

This comment has been minimized.

Copy link
Member Author

gkatsev commented Apr 4, 2018

no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.