[Discussion] More guidance / docs / programmatic help for switching between immutable vs. without (?)

[madjam002]

There seems to now be a dependency on the package immutable since #248, but it isn't listed in package.json dependencies.

Steps to reproduce:

$ create-react-app little-router-test
$ cd little-router-test
$ yarn add redux-little-router react-redux redux
$ echo "import { routerForBrowser } from 'redux-little-router'" > src/index.js
$ yarn build

Module not found: Can't resolve 'immutable' in 'redux-little-router/es/immutable/util'

Even though adding immutable to the package.json dependencies would fix this issue, isn't it better to make this an optional dependency? For those who want to opt in to using immutable support, they can import from redux-little-router/immutable or something, otherwise those who aren't using immutable.js are getting larger bundle sizes as immutable.js will be included in the bundle for no reason.

Cheers

[ryan-roemer]

redux-little-router handles immutable with a "try to import it, but it's totally cool if it's not found" approach. Any real import of immutable is guarded like https://github.com/FormidableLabs/redux-little-router/blob/35096478914d62f7be2c20d6110314bae8baa999/src/immutable/util/immutable.js#L15-L24

So what you have there is a warning that you can safely ignore if you aren't using immutable. I tweaked your code just a bit to be:

import { routerForBrowser } from 'redux-little-router'; // eslint-disable-line

console.log("TODO HERE", routerForBrowser);

And yarn build successfully exits 0 (even with the warning) as well as yarn start produces a working webpage with console.log as expected at: http://localhost:3000/

I'm going to close this issue, but if you want to say enhance the README.md with what would make this behavior less surprising for you in the future, we'd totally ❤️ that!

/cc @tptee

[madjam002]

Thanks for getting back so quickly.

Can you please re-open this issue as it hasn't been resolved, I wouldn't say that having warnings produced during build time by a direct dependency is acceptable behaviour. Some build systems won't allow for warnings, and I don't get warnings from any other package.

It seems like this has been done in a really strange way, usually if you wanted to have separate behaviour for a library like Immutable.js which is an optional dependency, you'd have a different import altogether so consumers who aren't using Immutable don't have extra code being bundled along with extra dependencies. I'd suggest that the Immutable helpers be imported from something like redux-little-router/immutable. This would be cleaner than having a try/catch around any import of Immutable.JS.

It's also worth noting that if I have Immutable.JS as a devDependency or if any other package depends on Immutable.JS, this method will still bundle it in my web app even though I'm not using it. A simple way to test this is to follow the code snippet I posted originally, and do:

$ yarn add -D immutable
$ yarn build
# Gzipped size +16Kb-ish

[ryan-roemer]

It's a try/catch on the require, so if it's present, it will end up in your bundle. You can alias this away in webpack to be something like throw Error() (or something like that) to make sure it doesn't.

Our goals here are:

1. Support immutable for folks who use it transparently.
2. Support folks who don't use immutable without increasing bundle size.

If you have any approach that can meet both these goals then I'll definitely re-open!

[ryan-roemer]

On second thought, re-titling and reopening as a discussion ticket.

[madjam002]

Can this just be split out to a different package? Or even just a separate import but from the same package like I suggested? redux-little-router/immutable.

Either way, it would be great if you could publish a patch to v14.x.x to undo this behaviour and republish as v15 as I think this is a breaking change? My CI builds have broken because of this, others will now be getting warnings depending on configurations.

[ryan-roemer]

And on the:

usually if you wanted to have separate behaviour for a library like Immutable.js which is an optional dependency, you'd have a different import altogether

vein of thought -- we tried this and it was really complicated and/or yielded a ton of awkward code duplication everywhere. If you have an elegant solution / a PR to make that seamless, we're more than all ears! (Especially given how many false start branches we had trying to get to the solution we finally came to :) )

[ryan-roemer]

@madjam002 -- If you want to take a stab at a PR to do this, we'd definitely love to see an elegant approach!

[ryan-roemer]

@madjam002 -- You can likely webpack alias away any warnings you're encountering in the meantime.

[madjam002]

What are your thoughts regarding rolling this back for the v14 branch for now and cutting a new v15 release?

Regarding a PR, I have no time currently to look at this unfortunately. I'm not prepared to invest time into something related to a feature that I don't use when v14.2 works just fine, so I'll lock down to that for now. I know I'm the only one reporting this issue for now so I think that's totally fair, if others have this issue though I think this is something that warrants further investigation.

[ryan-roemer]

@tptee -- If you have a moment, can you TAL and see if there's a reasonable rollback + upgrade cycle here?

Unfortunately, if we rollback v14.3.0 to remove immutable for v14.3.1 or v14.4.0 or whatever, we now have a legitimate breaking change for immutable-using folks, so we're going to impact either (1) people who have builds that fully break on mere warnings / don't have time to alias/bandaid away the warnings vs. (2) people who requested and now use the immutable support.

Or, we could just advise folks to pin to v14.2 and just make next release v15 or something. I'm not sure, so looking to your guidance here...

For anyone else following along, if anyone has a way to better make the warnings go away, I'm all ears!

For example, it might be something like refactoring to:

try { 
  immutable = require('immutable'); 
  if (immutable) {
    Map = immutable.Map; 
    List = immutable.List; 
    fromJS = immutable.fromJS; 
    // To account for immutable versions 3.8.x -> 4.x.x 
    isImmutable = immutable.isImmutable 
      ? immutable.isImmutable 
      : immutable.Iterable.isIterable; 
  }
} catch (e) {} 

and in webpack config do something to transform that require('immutable') to false. Or, in babel do the same.

[madjam002]

Ah yeah that's a good point. I'm happy to stick on v14.2 for now!

I still think that asking users to change config for this is a bit strange, a package should have a clean set of dependencies, "optional dependencies" can get a bit messy. What if there's another optional dependency added? All consuming projects need to update their config again?

Regarding the separate package idea, you mentioned that there was a tonne of code duplication, wouldn't it just be a case of removing all of the ./immutable import/exports from index.js and moving them to something like immutable/index.js and also reexporting everything from the base package in there? e.g export * from '../

That way people who don't want immutable.js can import from redux-little-router and those who want immutablejs can import from redux-little-router/immutable.

However you try and suppress the warnings by wrapping in a try/catch, that dependency will still get bundled if it has been installed!

[ryan-roemer]

Really digging in here, things may be simpler than I previously thought they were... And, I don't think modern builds should even be hitting this problem when configured for production.

@tptee -- Looking to src that isn't src/immutable. Is there something we could programmatically do to separate out the immutable re-exports to just redux-little-router/immutable?

@madjam002 -- What's weird is that if I'm reading things correctly, webpack tree-shaking should be removing all of the immutable source files completely. Only src/index.js imports from src/immutable and all those are real ESM re-exports. Are you using a modern webpack / rollup build config that has tree-shaking + dead code elimination included?

Update: Working on too many projects today -- this one has steps to repro with CRA. So it may be that CRA doesn't have a proper production webpack build :( -- digging in more (and may have to put on hiatus until next week)

[ryan-roemer]

Update

Ugh. 😿 So webpack tree-shaking at least as of v3 just doesn't work for root imports in redux-little-router/es/index.js. Basically, tl;dr -- you get a lot more code than you intend to bring in -- even using ESM the correct way as documented everywhere. This also explains the immutable issues here, and I'm leaving this issue open to make sure we resolve the immutable lib aspects on our way to a more general solution.

Full issue at: https://github.com/FormidableLabs/redux-little-router/issues/262

Sample repo at: https://github.com/FormidableLabs/rlr-tree-shaking-experiment

@madjam002 -- I have a workaround for current projects, which is to avoid redux-little-router/*/index.js-based imports and instead one-off import things you use from the full file paths. It's documented in the ticket, and I can help folks work out any kinks such as universal code (which requires more hacking).

This is obviously an unsatisfactory workaround, but right now, on any version of RLR, chances are very high that many projects are getting a lot more unused RLR-sourced code than they need to. (And I highly suspect the same goes for other common react libs).

[ryan-roemer]

We've done a breaking change to move the immutable methods to immutable/index.js instead of index.js. Given that webpack1-3 doesn't actually tree shake things out correctly, there is no other way to solve the "immutable library might accidentally end up in your bundle" problem in the interim before webpack4 is released and becomes ubiquitous.

So this means folks encountering this issue should just do a major bump -- given the only thing that changed was immutable stuff is moved, and no one here is using immutable, then should hopefully be easy!

We've also added enhancements so tree-shaking will remove stuff actually unused provided you're using (the still beta/next version) of webpack@4+.

https://github.com/FormidableLabs/redux-little-router/releases/tag/v15.0.0