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

refactor: use rollup for build #69

Merged
merged 7 commits into from Mar 30, 2018
Merged

Conversation

forbesjo
Copy link
Contributor

@forbesjo forbesjo commented Mar 27, 2018

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.

.babelrc Outdated
}
}]]
presets: ['es3', ['es2015', { loose: true, modules: false }]],
plugins: ['external-helpers', 'transform-object-assign']
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not preset env?

Copy link
Member

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.

Copy link
Member

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.

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 }]],
Copy link
Member

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.

@forbesjo forbesjo changed the title refactor: use rollup for build refactor: use rollup for build (WIP) Mar 27, 2018
@forbesjo forbesjo force-pushed the rollup_final branch 4 times, most recently from c4120bd to 1ca2d0c Compare March 28, 2018 14:29
@forbesjo forbesjo changed the title refactor: use rollup for build (WIP) refactor: use rollup for build Mar 28, 2018
output: [{
name: 'videojs-http-streaming',
file: 'dist/videojs-http-streaming.es.js',
format: 'es',
Copy link
Member

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


/**
* 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).
Copy link
Member

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.

Copy link
Contributor Author

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",
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@forbesjo
Copy link
Contributor Author

Requires a new version of the web worker plugin that has this change gkatsev/rollup-plugin-bundle-worker#1

@forbesjo forbesjo removed the blocked label Mar 30, 2018
@forbesjo forbesjo merged commit c28c25c into videojs:master Mar 30, 2018
@forbesjo forbesjo deleted the rollup_final branch March 30, 2018 17:48
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

Successfully merging this pull request may close these issues.

None yet

3 participants