Loadable-components: Fully dynamic loadable is not reactive

[luishdz1010]

🐛 Bug Report

Seems like only the very first prop sent to the loadable component in a dynamic context is respected, further updates to the props have no effect. Same applies to loadable.lib

To Reproduce

https://codesandbox.io/s/3wn6vo7ym

Expected behavior

Changing props will work reactively. An "easy" fix is to set a key prop to the loadable component, this works in the case of using a loadable component (although it seems like a hack) but for loadable.lib, this will trigger an unmount and remount of all children of the component, which is sometimes not desired as the rendered tree could be similar, you want to perform animations, etc.

## System:
 - OS: macOS 10.14.3
 - CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
 - Memory: 99.34 MB / 16.00 GB
 - Shell: 5.3 - /bin/zsh
## Binaries:
 - Node: 10.15.2 - /usr/local/opt/node@10/bin/node
 - Yarn: 1.15.2 - /usr/local/bin/yarn
 - npm: 6.4.1 - /usr/local/opt/node@10/bin/npm
 - Watchman: 4.9.0 - /usr/local/bin/watchman
## npmPackages:
 - @loadable/babel-plugin: ^5.7.2 => 5.7.2

[theKashey]

What is the use case?
Even is this _sounds_ like it should work - it's quite hard to distinguish props, which shall be forwarded to an underlying component, and the ones, which could change that component.

[gregberge]

Hello @luishdz1010, I see what you mean. I think we could try to fix it but if we did, you will have to specify dependencies like useEffect.

[luishdz1010]

My use-case is somewhat contrived. I'm using loadable.lib to load some data files, which represent data for different pages. Based on the current URL route, I load a different one. My issue is that different pages may use different wrapper components or "layouts", therefore the loadable has to be a topmost component in the render tree, since the loadable doesn't work reactively I have to resort to use key this works but re-mounts everything on page change, which prevents me from doing animations, keeping the <Layout> state below, and its way slower.

const PageJson = loadable.lib(props => import(`./pages/${props.path}`))

<PageJson key={location.pathname} path={location.pathname.replace(/\//, '_')}>
      {({
        default: {
          data
        },
      }) => (
        <Layout data={data.layout}>
          <Page data={data.page/>
        </Layout>
      )}
</PageJson>

@neoziro That seems like a sensible approach to me

[theKashey]

But what is the right behaviour:

• once dep is changed - drop the current state and load a new one. The same as key
• once dep is changes - load the new state and swap underlaying component. May lead to error, as long for a moment you will use and old one, like it's a new
• once dep is changes -load the new state, but continue using the old props to display an old one. This is more like transition-group is keeping _dead_ children for a while.

Only the second variant has to be implemented inside loadable components, while the rest are easy to wrap around.
Vote.

[luishdz1010]

@theKashey The first approach is already possible, but it does not have the same behavior as if it were implemented inside loadable. Changing the key treats the component as a totally different one, so an unmount/remount is guaranteed in all prop changes.

If we don't use key and let the loadable invalidate its own state after props change, it will most of the time also generate a unmount/mount cycle except in the case where new was previously loaded in a previous render cycle. In that specific case, the loadable will behave differently than usingkey because it will not unmount/remount its children, since it will simply pass new to the function-as-a-child, it is then the responsibility of the function-as-a-child to render whatever new/old component it wants.

This is what I'd expect, if I change the query in an Apollo's component I get a completely new data and loading cycle, and I'd argue conceptually the loadable.lib should work similar to in that regard.

[gregberge]

I think we could make it work the way you expect it to work. But it is a big change. I need time to work on it. For now if you don't need SSR a workaround is to create your own loadable.lib component.