-
Notifications
You must be signed in to change notification settings - Fork 421
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
refactor: use rollup for build #69
Conversation
.babelrc
Outdated
} | ||
}]] | ||
presets: ['es3', ['es2015', { loose: true, modules: false }]], | ||
plugins: ['external-helpers', 'transform-object-assign'] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not preset env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably could but since we're targeting IE11 and up, es2015 preset is kind of easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either one is fine. The babel team would probably prefer us to use the babel-preset-env, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easier for what?
especially because we are targeting IE11 and up if we use env and target browsers properly we can end up with less compiled features and build size will be smaller and work better (on supported browsers)
also env preset can decide if we need es3 stuff automatically by target browsers
.babelrc
Outdated
"browsers": ["last 2 versions", "ie 9-11"] | ||
} | ||
}]] | ||
presets: ['es3', ['es2015', { loose: true, modules: false }]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can drop the es3 preset.
c4120bd
to
1ca2d0c
Compare
scripts/rollup.config.js
Outdated
output: [{ | ||
name: 'videojs-http-streaming', | ||
file: 'dist/videojs-http-streaming.es.js', | ||
format: 'es', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably add an onwarn method here to ignore the unresolved dep warnings, see:
https://github.com/videojs/video.js/blob/378d98ee87b6ac48536d0b8f263bee1f76c68aab/build/rollup.js#L65-L73
scripts/rollup.config.js
Outdated
|
||
/** | ||
* Rollup configuration for packaging the plugin in a module that is consumable | ||
* by either CommonJS (e.g. Node or Browserify) or ECMAScript (e.g. Rollup). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esmodules work in webpack as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying the comment
"test/karma.conf.js", | ||
"scripts", | ||
"utils", | ||
"test/test-manifests.js" | ||
] | ||
}, | ||
"files": [ | ||
"dist/", | ||
"es5/" | ||
"dist/" | ||
], | ||
"dependencies": { | ||
"aes-decrypter": "1.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should upgrade to aes-decrypter 3 at some point, it is rolluped. I think/hope that 3 has the correct decrypter stuff and not the broken version that 2.0 had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We just need to verify it doesn't have the broken parts in it.
Requires a new version of the web worker plugin that has this change gkatsev/rollup-plugin-bundle-worker#1 |
This switches the build system from browserify to rollup to work around using webwackify and allows for easier integration into video.js.
This PR also removes the browserify/webpack tests as well as code coverage. Code coverage was causing the tests to fail, we'll revisit and investigate the problem.