Investigate tree-shaking

[markerikson]

As hooks continue to gain adoption, it's possible that some folks might want to write apps that only use our hooks API. I'm unsure whether connect actually tree-shakes or not.

It would be helpful if someone could do some testing to see if connect tree-shakes when only the hooks are used, and if it doesn't, figure out why not and what changes are needed to enable that. (On the flip side, I'd also be curious if the hooks tree-shake when you only use connect.)

[Andarist]

react-redux/src/index.js

Line 14 in 1186016

setBatch(batch)

Cannot be tree-shaken (unless a particular minifier goes deeper and figures out that this is needed only if set batch stays referenced), but it is expected. Would be cool to figure out how to do this lazily but it's a super minor thing.

react-redux/src/utils/shallowEqual.js

Line 1 in 1186016

const hasOwn = Object.prototype.hasOwnProperty

Top-level getter also cannot be tree-shaken, arguably this is a minor thing as well - but ideally, you should inline this into the function.

react-redux/src/utils/useIsomorphicLayoutEffect.js

Line 12 in 1186016

const isHopefullyDomEnvironment =

Similarly this - also a short leftover, but cannot be removed. A recommendation here would be making it a function, and put /* #__PURE__ */ right before the call:

const isHopefullyDomEnvironment = () =>
  typeof window !== 'undefined' &&
  typeof window.document !== 'undefined' &&
  typeof window.document.createElement !== 'undefined'

export const useIsomorphicLayoutEffect = /* #__PURE__ */ isHopefullyDomEnvironment()
  ? useLayoutEffect
  : useEffect

react-redux/src/hooks/useDispatch.js

Line 41 in 1186016

export const useDispatch = createDispatchHook()

Top-level function call prevents this from being tree-shaked. The recommendation would be a PURE annotation before the call.

react-redux/src/hooks/useSelector.js

Line 2 in 1186016

import invariant from 'invariant'

Most likely this is not written in ESM and this might prevent tree-shaking it. The recommendation would be to just migrate to using if+throw explicitly without somewhat quirky API wrapper - the React team is doing the same thing from what I read (migrating away from this pattern).

react-redux/src/hooks/useSelector.js

Line 134 in 1186016

export const useSelector = createSelectorHook()

Similar to useDispatch - PURE annotation should do its trick here.

react-redux/src/hooks/useStore.js

Line 37 in 1186016

export const useStore = createStoreHook()

The same thing, just for the useStore this time.

react-redux/src/components/Context.js

Lines 3 to 5 in 1186016

	export const ReactReduxContext = React.createContext(null)
	ReactReduxContext.displayName = 'ReactRedux'

Both of those (createContext call and displayName assignment) cannot be removed. The first one should be fixable with a PURE annotation and for the latter, I would recommend wrapping it with a NODE_ENV check - it's not that much needed in prod build anyway, right?

react-redux/src/components/Provider.js

Line 36 in 1186016

Provider.propTypes = {

Statics assignment is one of the biggest tree-shaking offenders. I would recommend wrapping this with NODE_ENV check as well - as those are most certainly not needed at all in production.

react-redux/src/components/connectAdvanced.js

Line 1 in 1186016

import hoistStatics from 'hoist-non-react-statics'

Bundlers might have a little bit hard time of pruning this as its CJS - I've recommended providing ESM ("module") build there, but it was ignored/declined. Maybe they could at least add "sideEffects": false to their pkg.json - which at least would have the potential for removing this in webpack's case.

react-redux/src/components/connectAdvanced.js

Line 4 in 1186016

import { isValidElementType, isContextConsumer } from 'react-is'

react-is is also CJS-only and rather not treeshakeable right now. There is an open PR for improving the situation in this regard (facebook/react#13321) but it got stale - I've offered help to finish it sometime this week but I haven't received any answer yet.

react-redux/src/connect/connect.js

Line 104 in 1186016

export default createConnect()

Yet Another Top Level Call 😉 PURE annotation should help.

Summary
Few PURE annotations should improve tree-shakeability. At the same time having some of the current dependencies makes it hard/impossible for tree-shaking to prune all of the unused stuff.

There are also few other, minor, things that could be improved - for those that I consider easy and no-brainers I can prepare a PRs with numbers before/after, just let me know if you want it.

[markerikson]

@Andarist : fantastic analysis, thank you!

Yes, I'd be interested in any PRs that can improve the tree-shakability overall. It would be helpful if we could have some sample projects set up somewhere that would demonstrate the before-and-after results to confirm the behavior improvements, as well.

[Andarist]

IMHO the best way to test tree-shakeability is to literally try to build this with webpack & rollup:

import 'redux'

I can prepare a simple repro that would contain setup for both of those and we could observe on it how we improve the situation with each change - starting from less controversial ones to those a little bit more controversial and finally landing with a list of unresolved stuff that we could discuss further.

Expect a first PR over the weekend.

[mreishus]

I set up a quick bash script which makes 3 create-react-apps, installs redux and react-redux, and creates 3 version of the sample todo app, connect only, hooks only, and connect+hooks.

See: https://github.com/mreishus/react-redux-tree-shake

[timdorr]

react-redux/src/utils/shallowEqual.js

Line 1 in 1186016

const hasOwn = Object.prototype.hasOwnProperty

Top-level getter also cannot be tree-shaken, arguably this is a minor thing as well - but ideally, you should inline this into the function.

Fixed.

react-redux/src/utils/useIsomorphicLayoutEffect.js

Line 12 in 1186016

const isHopefullyDomEnvironment =

Similarly this - also a short leftover, but cannot be removed. A recommendation here would be making it a function, and put /* #__PURE__ */ right before the call:

const isHopefullyDomEnvironment = () =>
  typeof window !== 'undefined' &&
  typeof window.document !== 'undefined' &&
  typeof window.document.createElement !== 'undefined'

export const useIsomorphicLayoutEffect = /* #__PURE__ */ isHopefullyDomEnvironment()
  ? useLayoutEffect
  : useEffect

Fixed.

react-redux/src/hooks/useDispatch.js

Line 41 in 1186016

export const useDispatch = createDispatchHook()

Top-level function call prevents this from being tree-shaked. The recommendation would be a PURE annotation before the call.

/*#__PURE__*/ added here and a few other places.

react-redux/src/components/Context.js

Lines 3 to 5 in 1186016

	export const ReactReduxContext = React.createContext(null)
	ReactReduxContext.displayName = 'ReactRedux'

Both of those (createContext call and displayName assignment) cannot be removed. The first one should be fixable with a PURE annotation and for the latter, I would recommend wrapping it with a NODE_ENV check - it's not that much needed in prod build anyway, right?

These will never get removed anyways. You cannot make use of this library without context (all the hooks use it).

react-redux/src/components/Provider.js

Line 36 in 1186016

Provider.propTypes = {

Statics assignment is one of the biggest tree-shaking offenders. I would recommend wrapping this with NODE_ENV check as well - as those are most certainly not needed at all in production.

Yup, we should add dev mode checks. We do this over on React Router with a __DEV__ check (which we use babel-plugin-dev-expression for):

https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/Route.js#L81

react-redux/src/components/connectAdvanced.js

Line 1 in 1186016

import hoistStatics from 'hoist-non-react-statics'

Bundlers might have a little bit hard time of pruning this as its CJS - I've recommended providing ESM ("module") build there, but it was ignored/declined. Maybe they could at least add "sideEffects": false to their pkg.json - which at least would have the potential for removing this in webpack's case.

react-redux/src/components/connectAdvanced.js

Line 4 in 1186016

import { isValidElementType, isContextConsumer } from 'react-is'

react-is is also CJS-only and rather not treeshakeable right now. There is an open PR for improving the situation in this regard (facebook/react#13321) but it got stale - I've offered help to finish it sometime this week but I haven't received any answer yet.

Nothing we can really do about external packages. If there are lighter alternatives available, we can switch.

[Andarist]

I've set up my tree-shaking playground for react-redux and you can check out how those PURE annotations helped:
Andarist/react-redux-tree-shaking-playground@cee212d#diff-b027168eed2f9d69319d4819454b8ab4

This yields tremendous improvements (%-speaking).

useIsomorphicLayoutEffect

@timdorr this was not quite the change I had in mind. The deopting pattern is getter used during script's initialization and not assigning to a one-time variable. This lone stays in the bundle after changes in case of webpack:
https://github.com/Andarist/react-redux-tree-shaking-playground/blob/cee212dfec0845f12f9b0e464505c64d17ab21fa/dist/webpacked.js#L379
Rollup seems to remove it - I guess it has a whitelist of global pure getters.

These will never get removed anyways. You cannot make use of this library without context (all the hooks use it).

Yes, you might not be able to use this library without context BUT the library might get referenced indirectly and might stay unused - in such case, in ideal world, it would just vanish completely, along with this React.createContext call. I understand this is a less common scenario - but it exists, for simplicity, you might decide that you don't want to care about it of course.

Nothing we can really do about external packages. If there are lighter alternatives available, we can switch.

Those points were rather a general remark that as long as they stay as your dependencies they might not get removed (because of how they are written/bundled - not because it's impossible to remove their code if it would be structured other way).

EDIT:// After looking closer into current results - when using Rollup there is barely any react-redux code left in the bundle 🎉 it's mostly react-is, prop-types & hoist-non-react-statics. The situation is less ideal with webpack - but I hope to dig further to see how the situation can be optimized.

[markerikson]

So my other main question about this, besides all these smaller optimizations:

Do connect and hooks shake out when only one of them is used?

[Andarist]

Right now (based on the master) when nothing gets used we end up with:

• getBatch
• Subscription
• ReactReduxContext
• Provider
• useIsomorphicLayoutEffect
• randomString (I guess it's from redux)

My PR here - #1471 - removes further:

• getBatch
• Subscription
• ReactReduxContext
• Provider

Leaving you with:

• useIsomorphicLayoutEffect
• randomString (I guess it's from redux)

And not-shaked code from your CJS deps.

[timdorr]

Yes, you might not be able to use this library without context BUT the library might get referenced indirectly and might stay unused - in such case, in ideal world, it would just vanish completely, along with this React.createContext call. I understand this is a less common scenario - but it exists, for simplicity, you might decide that you don't want to care about it of course.

How would it get referenced indirectly? I'm trying to think of the practical implications here. No one is doing import 'react-redux'. At worst (and still very unlikely), they are doing import * as ReactRedux from 'react-redux' and are making use of something that references context in some way. I don't know of a code path that would avoid it completely.

I think the more practical example here is to check sizes when you do import {useSelector, useDispatch, useStore} from 'react-redux' (or just useSelector), as that was Mark's original concern. You should be getting as minimal of a bundle as possible when using those imports.

[Andarist]

How would it get referenced indirectly? I'm trying to think of the practical implications here.

Like when you have a dependency A depending on react-redux and bunch of other stuff, but you import from it only stuff unrelated to react-redux. I understand this is unlikely in react-redux case, but I try to present all the possibilities - as if we are able to just tree-shake everything (ideal situation) we don't need to do any assessment of "does this particular optimization thing matter?". We just try to reach the ideal state and if we fail then we can assess what kind of leftovers we have and how much they matter.

I think the more practical example here is to check sizes when you do import {useSelector, useDispatch, useStore} from 'react-redux' (or just useSelector), as that was Mark's original concern. You should be getting as minimal of a bundle as possible when using those imports.

IMHO that's a separate test. Solely for testing tree-shaking, it's just better to check that import 'react-redux' because it immediately can reveal everything that gets left out after tree-shaking takes place - without complicating it with different scenarios like:

• what if we only include A
• what if we include B & C
• etc

What you are proposing is a bundle size check but with such, it would be harder to assess if there is in the bundle anything that shouldn't be there because you would have to read outputs carefully. With my simple test we only have to inspect the output and see if anything is still there.

[timdorr]

No, I am not talking about testing different import combinations. I'm only talking about testing Hooks-only usage. That was the reason Mark opened this issue.

I don't want to waste your time testing an unrealistic scenario where someone imports the library but doesn't use it. That's not really going to benefit anyone if the deopts kick in as soon as you import something for real.

We've basically got two sections of the library: HOC and Hooks. If you're a HOC-only user, Hooks shouldn't show up in your code. And vice versa for Hooks-only users. While there are going to be a lot of mixed users, we should still provide a clean export for those that can keep it to one or the other.

It looks like we've made some progress by hinting that the Hook factory exports are pure. And those other PRs have helped too. So can we see where we're at for either the Hooks- and/or HOC-only use case?

[Andarist]

I don't want to waste your time testing an unrealistic scenario where someone imports the library but doesn't use it. That's not really going to benefit anyone if the deopts kick in as soon as you import something for real.

My point is that this really doesn't matter here - if we e.g. connect gets removed successfully when we don't import anything then it won't be included either if we import unrelated export. Unless ofc such imported export would depend on connect, but we don't need any tests to be rather sure that it doesn't.

All of this is based on static analysis of the code, side effects created by expressions and dependencies between particular values. Dependencies conceptually form a graph and if we are able to prune a particular subtree of it and if we include a separate branch of that tree by importing stuff it doesn't change anything for that pruned subtree.

We can prepare another test that would actually import something but that just wouldn't change the outcome, could only act as last step verification if we understand how this all works like we think it does. In such a case its purpose would be, in my opinion, different than testing if tree-shaking works here, because that is already showcased by existing "test" (https://github.com/Andarist/react-redux-tree-shaking-playground).