Skip to content

Fix bug where calling load() on lazy element breaks the loading #66

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

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

latentflip
Copy link

@latentflip latentflip commented Jul 29, 2021

Fixes #67

This PR fixes #67 where calling load() on a loading=lazy element causes the contents not to be replaced when the element becomes visible.

Context

There are two bugs at play here:

  1. We were calling observer.unobserve when load() was called. This is incorrect as it should only be called once content replacement has happened.
  2. load() doesn't actually properly preload the request, since nothing is done with the response - it should be saved into the weakmap but that is only done by getData() not load().

Fix

The fix here is twofold:

  1. Remove the observer.unobserve call from the load() code.
  2. Move load() logic to a private function and instead call getData from the element.load() function, getData correctly caches the response or immediately returns a cached response, so it is the correct code to call to preload the response#66

Implications

Technically this change does modify the public api of include-fragment - if someone was subclassing the element and re-implementing load() to modify say how events are fired, this will no longer work for them. I cannot find any examples of us doing this within github (we do reimplement fetch but not load.

@latentflip latentflip marked this pull request as ready for review July 29, 2021 10:51
@latentflip latentflip requested a review from a team as a code owner July 29, 2021 10:51
@latentflip latentflip changed the title Bug where calling load() on lazy element breaks the loading Fix bug where calling load() on lazy element breaks the loading Jul 29, 2021
Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me ✨

@koddsson koddsson merged commit 54caf8e into main Jul 29, 2021
@koddsson koddsson deleted the preload-bug branch July 29, 2021 11:33
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.

Calling load() on a lazy loaded element doesn't work
2 participants