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

Bundling & minifying concerns #5

Closed
oyeanuj opened this issue Oct 29, 2017 · 7 comments
Closed

Bundling & minifying concerns #5

oyeanuj opened this issue Oct 29, 2017 · 7 comments

Comments

@oyeanuj
Copy link
Contributor

oyeanuj commented Oct 29, 2017

Hi @mrmartineau, this is more of a question/concern than a definitive issue. I've been testing including this in the production bundle and I noticed that just importing the DesignSystem increases my bundle size by 500K which is unusual.

Looking at the package.json, it seems like maybe more than just the compiled index.js is being included, and instead having a compiled index.js as the only thing that the package.json is linked to would help?

So, I was wondering if you have seen this issue as well? Or have any ideas on what might be going on?

FWIW, I was testing with this starter kit (which is what my setup is based on) incase you wanted to try it out as well: https://github.com/ctrlplusb/react-universally

Thank you!

@mrmartineau
Copy link
Owner

@oyeanuj thanks for your concern, the footprint of this bundle should be very small. There maybe a bundling issue that meant it is larger than it should be, but I'm pretty sure there isn't because when I look at the cjs (dist) directory, it looks like this unminified: https://www.dropbox.com/s/trc6eahgxnrp3oz/Screenshot%202017-10-30%2007.21.13.png?dl=0

There are two dependencies:

  • modularscale-js: 966 bytes minified (505 bytes gzipped)
  • object-get: 740 bytes minified (421 bytes gzipped)

Now I don't know how large your design system object is, but that might be the issue..

If there's something that I'm missing, it would be great to understand what it is so we can resolve this.

@mrmartineau
Copy link
Owner

@oyeanuj are you certain that this package increased your bundle by 500k? Have you had a chance to test it?

Also, watch this space for some updates. There's an open PR at the moment, that changes the API slightly, and I will be working on some of your other suggestions soon.

@oyeanuj
Copy link
Contributor Author

oyeanuj commented Nov 3, 2017

@mrmartineau I do agree that is weird, hence the framing as a question. So, I've tried to reproduce it in a couple of ways:

  1. Create React App: This failed to compile the production build as it couldn't find a pre-compiled file in the repo. Here is the reproduction (https://github.com/oyeanuj/my-errors/tree/design-system-utils) and here was the errors:
→ yarn run build
yarn run v1.3.2
$ react-scripts build
Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file:

 	./node_modules/design-system-utils/src/index.js:6

Read more here: http://bit.ly/2tRViJ9

error Command failed with exit code 1.

So that might be something to fix in general?

  1. With React-Universally: I've been able to reproduce it with a clean branch from the master of the kit. If you run yarn run build before and after this one commit, you see the weird increase in bundle size.

I'll try to dig more to isolate the cause to see if it was my setup or can be reproduced generally (hence the attempt to use CRA originally).

@mrmartineau
Copy link
Owner

This is a strange one... Bundlephobia says the package is 887b

Regarding the minify error. I am using this info in the package.json, which means webpack will try to use that if it can.. so perhaps your webpack config needs amending.. I realise this isn't a solution for everyone so it would be good to fix.

"main": "./cjs/index.js",
"module": "./src/index.js",

@oyeanuj
Copy link
Contributor Author

oyeanuj commented Nov 3, 2017

On more investigation, it does seem to be a minification error of some sort while bundling. I imported the files in the package to use them as a local library and the error went away.

I imagine it has to do something with UglifyJS not working well with this library for some reason. If it is unable to minify and uglify, that could explain the bundle size increase. Both my config (via React-Universally) and CRA use Uglify and both are having those issues.

It seems like we should resolve the CRA minify issue above as per http://bit.ly/2tRViJ9 since that would help a lot more people? Not sure what is the best way to do so.

@oyeanuj oyeanuj changed the title Bundle size increase concern Bundling & minifying concerns Nov 3, 2017
@oyeanuj
Copy link
Contributor Author

oyeanuj commented Nov 3, 2017

Another update: So, it turns out that Webpack v3 so far uses the 0.4.6 of the UglifyJS plugin and with v4, they will start using 1.x of the UglifyJS plugin (which internally uses Uglify-ES).

So, I upgraded to v1 and the errors seem to go away, and the bundle size seems back to normal.

Closing issue to reduce noise now that we know the cause.

PS. @mrmartineau thanks for engaging in this conversation!

@oyeanuj oyeanuj closed this as completed Nov 3, 2017
mrmartineau added a commit that referenced this issue Nov 5, 2017
mrmartineau added a commit that referenced this issue Nov 5, 2017
@mrmartineau
Copy link
Owner

@oyeanuj thanks for looking into this again. I'm not sure of the best way to proceed. I think it could be due to the main/module fields, perhaps I also need a browser field as well (See https://webpack.js.org/configuration/resolve/#resolve-mainfields for more info), but even that probably wouldn't solve the issue because that bundle ought to be a UMD version.. Any ideas?

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

No branches or pull requests

2 participants