-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add support for bundling using Rollup #59
Conversation
/** | ||
* @param {import('../configure').Options} options | ||
*/ | ||
export function babelConfig(options) { |
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.
This file is basically the same as babel-loader.js except I separated the config part from the loader part so I could re-use it in rollup.
As usual, this is fantastic work 🙌
|
Nice! Rollup does provide an API to modify the config, but plugin "options" are already consumed at that point (since they're just parameters to functions not called by rollup): rollup.rollup({
plugins: [
{
name: 'modify-config-plugin',
options(inputOptions) {
inputOptions.plugins[0] // instance of the plugin
}
}
]
}); |
a0a96fc
to
8e35e3b
Compare
Okay, I think this is ready to go! The rollup testing is a little weird since we include rollup in karmatic as a devDep through microbundle, so technically the tests run using that version of rollup and not the one specified in the test package.json but oh well 🤷♂️ - it's good enough I think. Was still useful for me to catch some bugs in properly loading rollup configs. |
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.
Other than the suggestion I made in chat about having karmatic ship with rollup
as a dependency by default and falling back to then when there's no rollup
/webpack
install present, everything here looks awesome.
Co-authored-by: Jason Miller <developit@users.noreply.github.com>
Couple of decisions I made up I wanted to call out:
I decided to include
node-resolve
andcommonjs
plugins as deps so our default rollup config is mildly useful. Was thinking for uses withmicrobundle
where there isn't arollup.config.js
for us to use. Would be curious to get other's thoughts here on what plugins we should include.If a rollup config is present, then I just use that unmodified. I'm not sure if rollup provides an API to modify the config/options of plugin instances from a config object. I think this method (of just using the provided config unmodified) is fine though. I think the only thing lost right now is code coverage (though I guess I could just add another babel plugin instance with only the coverage plugin configured...)
This is still a work in progress but thought I'd put an early draft out here to get some feedback. I also need to update the testing infra a bit so I can validate this implementation but that's gonna come in a different PR.