Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

rxjs 5 tree shaking #422

Closed
wants to merge 2 commits into from
Closed

rxjs 5 tree shaking #422

wants to merge 2 commits into from

Conversation

rafaelkallis
Copy link

Currently, the rxjs config of recompose, for the mapStreamToProps and componentFromStream enhancers, imports the whole rxjs library. This is quite bad for size-sensitive builds.

As described here, it is possible to selectively import the features we need from rxjs in order to reduce the build size.

In this PR, I have changed the rxjs 5 config such that only necessary features get imported, dramatically decreasing build size for apps using the enhancers mentioned above with rxjs 5.

In my current app, I have reduced the production build size by 100KB with this change.

@rafaelkallis
Copy link
Author

rafaelkallis commented Jul 4, 2017

@acdlite question regarding the travis config, isn't a yarn install necessary? Or am I missing something obvious here? I believe that's the reason the travis checks are failing.

The tests are just fine when running on my local machine.

@TrySound
Copy link
Contributor

TrySound commented Jul 4, 2017

@rafaelkallis I guess it's because observable directory is not capitalized in this package.

@istarkov
Copy link
Contributor

istarkov commented Jul 4, 2017

Thank you.
Just from is useless, then users need to use bind operator or add other rx operators via global import. Everyone can create and use his own config if he need to reduce app size, or use smaller rx alternatives.

@codecov-io
Copy link

codecov-io commented Jul 5, 2017

Codecov Report

Merging #422 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #422   +/-   ##
======================================
  Coverage    88.8%   88.8%           
======================================
  Files          51      51           
  Lines         375     375           
======================================
  Hits          333     333           
  Misses         42      42
Impacted Files Coverage Δ
src/packages/recompose/rxjsObservableConfig.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00f4316...4585690. Read the comment docs.

@rafaelkallis
Copy link
Author

rafaelkallis commented Jul 5, 2017

@TrySound thanks for the tip, seems to work now.

@istarkov thanks for the feedback. The uselessness of only importing from is relative to the use-case. At least it should be mentioned in the docs that the rxjsConfig imports the whole rxjs lib and therefore is not suitable for size-sensitive builds. We could also make a separate rxjsLiteConfig. What are your thoughts?

@istarkov
Copy link
Contributor

istarkov commented Jul 5, 2017

I think that change of documentation is ok,
I thinks that we can place a link to gist with rxjsLiteConfig there too, as there are a lot of cases where we need an additional config, as an example SSR config to avoid memory leaks #328 (comment)
Having that every library can have similar problems we could get enormous amount of configs ;-) we need to support
I'll approve such PR

@wuct
Copy link
Contributor

wuct commented Jul 6, 2017

Having that every library can have similar problems we could get enormous amount of configs ;-) we need to support

In that case, I think we should create a dedicated page for them.

@rafaelkallis
Copy link
Author

@istarkov sounds reasonable to me, PR can be closed in that case.

@istarkov istarkov closed this Jul 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants