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

Cache promise on load/preload #830

Merged
merged 1 commit into from
Dec 12, 2021
Merged

Conversation

macko911
Copy link
Contributor

Summary

I guess the cache in preload and load methods is not used on purpose but I was curious whether you'd be interested in this change, maybe under an option or a different static method cache.

The use case for me is that I did try to prefetch pages on link hover, but after transiting to the prefetched page I saw a blink of white page in between the route transitions, even though I was blocking the transition until the module was fetched.

This only happened on the first transition, subsequent transitions to the cached pages didn't have the blink anymore, which lead me to this change.

@theKashey theKashey self-assigned this Sep 15, 2021
@cecchi
Copy link

cecchi commented Nov 3, 2021

Hey @theKashey -- is this ready to go? Not sure I understood the review comment, but happy to help with implementation if I can. We're running into this issue as well.

Edit: looking again, is the suggestion to reuse the cached promise if one exists? In other words, only assign to cache[getCacheKey()] if the cache is not already populated?

@theKashey
Copy link
Collaborator

Everything is fine, I am just have developed a habit to always seek for perfection as mentoring our devs is a big part of my job.
And that is not the best habit.

gonna merge this PR and apply changes I would like to see by myself, that’s just a few lines change.

1 similar comment
@theKashey
Copy link
Collaborator

Everything is fine, I am just have developed a habit to always seek for perfection as mentoring our devs is a big part of my job.
And that is not the best habit.

gonna merge this PR and apply changes I would like to see by myself, that’s just a few lines change.

@macko911
Copy link
Contributor Author

macko911 commented Nov 9, 2021

Thanks @theKashey, I'd do the change myself but unfortunately don't really know what you meant by your original comment. Probably better to just do it yourself like you said 👍 . Would be really glad to stop using my fork of the repo. Cheers!

@theKashey theKashey merged commit 33cb54d into gregberge:main Dec 12, 2021
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.

3 participants