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

Add support for bundling using Rollup #59

Merged
merged 17 commits into from
Aug 15, 2020

Conversation

andrewiggins
Copy link
Collaborator

Couple of decisions I made up I wanted to call out:

  1. I decided to include node-resolve and commonjs plugins as deps so our default rollup config is mildly useful. Was thinking for uses with microbundle where there isn't a rollup.config.js for us to use. Would be curious to get other's thoughts here on what plugins we should include.

  2. 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.

/**
* @param {import('../configure').Options} options
*/
export function babelConfig(options) {
Copy link
Collaborator Author

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.

src/rollup.js Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Collaborator

As usual, this is fantastic work 🙌

  1. Agree, I'm in favor of adding both @rollup/plugin-node-resolve and @rollup/plugin-commonjs. Those are very much needed in nearly every project anyway.

  2. Never seen such an API, too. The docs don't seem to mention anything like that.

@developit
Copy link
Owner

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
            }
        }
    ]
});

@andrewiggins
Copy link
Collaborator Author

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.

@andrewiggins andrewiggins marked this pull request as ready for review August 14, 2020 05:26
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@developit developit left a 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.

andrewiggins and others added 2 commits August 14, 2020 14:10
Co-authored-by: Jason Miller <developit@users.noreply.github.com>
@marvinhagemeister marvinhagemeister merged commit 4346b0b into developit:master Aug 15, 2020
@andrewiggins andrewiggins deleted the add-rollup branch August 15, 2020 14:40
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.

3 participants