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

feat: support reactive dynamic loadable #330

Merged
merged 1 commit into from
May 12, 2019
Merged

Conversation

gregberge
Copy link
Owner

@gregberge gregberge commented May 8, 2019

Closes #284

@gregberge
Copy link
Owner Author

@theKashey could you review it please?

@theKashey
Copy link
Collaborator

I would like to make it a bit more complex:

  • an option to display old Component while a new one is loading. It could be an option to a LoadingComponent. The problem - right now you are loosing cacheKey and unable to refer to a prev data
  • if cacheKey got changed during loading - it would not cancel loadAsync and it would update component. You have to add more checks (more about store ref to a promise created, and compare it with the current one).
  • theoretically you shall not display old component with a new props, if a new component should be displayed instead. What about using getDerivedStateFromProps to derive state.props from props, and freeze them, if state.usedCacheKey!==state.cacheKey.

This would be needed only for .1, as long as it's much easier to add some cache-key logic in a render (still having old cache-key in a state). But, in the same time, this logic would ruin all the structure, unless implemented in hooks.

Oh, it really could become a bit complicated.

@gregberge
Copy link
Owner Author

an option to display old Component while a new one is loading. It could be an option to a LoadingComponent. The problem - right now you are loosing cacheKey and unable to refer to a prev data

It is not the responsibility of this component, it could be done in userland I think.

if cacheKey got changed during loading - it would not cancel loadAsync and it would update component. You have to add more checks (more about store ref to a promise created, and compare it with the current one).

Yeah you are right!

theoretically you shall not display old component with a new props, if a new component should be displayed instead. What about using getDerivedStateFromProps to derive state.props from props, and freeze them, if state.usedCacheKey!==state.cacheKey.

Yes you are right, I think getDerivedStateFromProps is a good workaround.

@gregberge gregberge merged commit 5a1fa73 into master May 12, 2019
@gregberge gregberge deleted the reactive-dynamic-loadable branch May 12, 2019 17:37
gregberge added a commit that referenced this pull request May 12, 2019
@luishdz1010
Copy link

Glad this got in!

I'm trying to implement keeping the old component mounted while loading in userland and it works fine, however one issue I cannot seem to move past is that there is always a loading phase, even if both components have already loaded.

I'd expect a seamless switch between them, like:

CompB -> CompB

But currently it goes:

CompA -> fallback (loading === true) -> CompB

Even if both CompA and CompB were previously loaded. If using loadable.lib, then the children-as-a-function prop gets "skipped" for a brief moment until the next render (which happens immediately). This causes problems with animations because it unmounts/mounts the whole tree under loadable.lib.

Just wanted to point this out for future readers seeking a solution to this, I understand fixing this may be a bit more complex than acceptable.

@gregberge
Copy link
Owner Author

Hello @luishdz1010, for now, yes it will switch to loading state for a very small time. But even if it was not the case, the component will still be mounted / unmounted and it will not solve your animation problem.

To display the previous component during loading, the strategy will be the same as a Transition Group. You have to check when the component will change and keep displaying the previous one until the new one is ready (fallback has disappeared).

fivethreeo pushed a commit to fivethreeo/loadable-components that referenced this pull request Nov 16, 2022
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

Successfully merging this pull request may close these issues.

Fully dynamic loadable is not reactive
3 participants