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

New dependency react-spinners increases build size #202

Closed
apepper opened this issue Jan 31, 2018 · 10 comments
Closed

New dependency react-spinners increases build size #202

apepper opened this issue Jan 31, 2018 · 10 comments

Comments

@apepper
Copy link
Contributor

apepper commented Jan 31, 2018

I tried out react-images in version 0.5.15 and 0.5.16. Compared to 0.5.14 the resulting bundle size increase alot, adding ~90 kb of uglified, but not-gzipped payload.

One way to measure it, is this way:

Steps to reproduce the behavior:

  • Call yarn webpack --progress -p

Expected behavior:

app.js should not be too big (ideally around ~190 kb, as with 3d48f47)

Actual behavior:

app.js is quite big (currently ~277 kb, as with 7f229b1)

It appears, that the biggest difference between those versions is #187. That PR added a new dependency to http://www.davidhu.io/react-spinners/.

@jorrit
Copy link
Contributor

jorrit commented Jan 31, 2018

It appears that all spinners are included in the resulting build. It's just the example. The downside is that the examples are not excluded from the NPM package.

@apepper
Copy link
Contributor Author

apepper commented Jan 31, 2018

I know, that this is just the example. But I do see about the same size increase in a production app, that tried to use the newer react-images version. But since it adds another 100 kb payload we are sticking to 0.5.14 for now.

@jorrit
Copy link
Contributor

jorrit commented Jan 31, 2018

Ah, that is awful. It seems that it is hard to import a single spinner from react-spinners using rollup without using code like const BounceLoader = require('react-spinners/dist/spinners/BounceLoader').default; instead of an ES6 import.

@neptunian
Copy link
Collaborator

neptunian commented Jan 31, 2018

Perhaps there is a way to remove react-spinners as a dependency and use it in your own app, so its only in the examples app. @mkalygin is that at all possible? Then at some point we should separate out the demos and have a simpler example in the build and the more advanced/detailed ones on the github site. No need for the user to have to download an entire website with the package.

@jossmac
Copy link
Owner

jossmac commented Feb 1, 2018

Whoa, that's massive! Though shouldn't be a problem for too much longer.

@apepper @jorrit please see #199 -- i've rewritten the lib to allow consumers the ability to implement their own components. e.g. a custom view with spinner for loading

apepper added a commit to Scrivito/scrivito_example_app_js that referenced this issue Feb 1, 2018
@mkalygin
Copy link
Contributor

mkalygin commented Feb 1, 2018

@jossmac @neptunian I think it's possible. Actually it already supports custom spinners. So we can safely remove the react-spinners dependency and provide a better documentation on how to add a basic custom spinner in examples.

What do you think? I'll be able to implement this. Or maybe I could help with the v1.0 release.

@neptunian
Copy link
Collaborator

Not sure when v1 is going to be ready so maybe best to take care of it now. I'm okay with it being removed and only existing as documentation. Ideally i'd like to have the website separated and this included, but for now, perhaps its best to keep it light.

@mkalygin
Copy link
Contributor

mkalygin commented Feb 2, 2018

@neptunian sounds good, I'll be able to solve it this weekend.

mkalygin added a commit to mkalygin/react-images that referenced this issue Feb 6, 2018
mkalygin added a commit to mkalygin/react-images that referenced this issue Feb 6, 2018
@mkalygin
Copy link
Contributor

mkalygin commented Feb 6, 2018

@apepper @jorrit @neptunian

I've removed react-spinners dependency and implemented default spinner in #208. You can also implement your own custom spinner, see examples. 😃

@jossmac jossmac closed this as completed in a42ffca Feb 7, 2018
@apepper
Copy link
Contributor Author

apepper commented Feb 8, 2018

Thanks to everyone involved fixing this! 👏

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

5 participants