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

Stop bundling external dependencies for cjs and esm bundles #513

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Stop bundling external dependencies for cjs and esm bundles #513

merged 1 commit into from
Mar 14, 2018

Conversation

vlad-zhukov
Copy link
Contributor

Currently formik bundles contain all external dependencies, which add unnecessary KBs to final app bundles.

Before: ~190KB
After: ~43KB

@jaredpalmer
Copy link
Owner

Maybe i don't understand, but at this point we might as well move all deps to peerDeps yes?

Copy link

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

Seems good to me 👍

As a side note while we are at optimizing things - any reason why formik includes it's own copy of hoistStatics? I have not compared it with original one, but seems to be 100% the same (except the copy is written in TS). You could probably shave some more bytes by removing this custom version and use the original one ( https://github.com/mridgway/hoist-non-react-statics ), cc @jaredpalmer

@Andarist
Copy link

Maybe i don't understand, but at this point we might as well move all deps to peerDeps yes?

Not really, deps will be installed by consumers automatically while peerDeps won't - which is obvious, so I guess I might have misunderstood your intent.

Anyway - the point of this PR is to keep deps as deps and let npm/yarn dedupe those dependencies when installing multiple packages, by bundling them into the source code you effectively prevent that.

@jaredpalmer
Copy link
Owner

The ts types were whack and I couldn't figure them out initially, was easier to write in TS, when I last checked like 6 months ago

@jaredpalmer jaredpalmer merged commit 9ec072a into jaredpalmer:master Mar 14, 2018
@jaredpalmer
Copy link
Owner

jaredpalmer commented Mar 14, 2018

@Andarist what's needed to solve the microbundle x prop-types issue?

@vlad-zhukov vlad-zhukov deleted the stop-bundling-deps branch March 14, 2018 19:59
@Andarist
Copy link

I'm not yet sure - need to investigate why it happens (how rollup-plugin-commonjs works) and maybe we could fix it upstream in the plugin - if not we can always hardcode "the fix" in microbundle's code.

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.

4 participants