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

[LiveComponent] Fix live component rendering when loaded from bfcache #436

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

1ed
Copy link
Contributor

@1ed 1ed commented Aug 20, 2022

Q A
Bug fix? yes
New feature? no
Tickets #510
License MIT

I have a live component with a form. On a fresh initial page load it works normally, but when I submit the form (not with ajax) and the controller redirects to another page and I come back using the browsers back button the live forms stops working. It does not re-render because of this check

// check if the page is navigating away
if (this.isWindowUnloaded) {
return;
}

If I comment it out, or I enable Turbo, it works. I think this is because bfcache of the browser. If I change the beforeunload listener to unload it works too, but that disables the cache.

Why we need this check? Can we leave it out? If we really need it we can use pagehide and pageshow instead. See https://webkit.org/blog/516/webkit-page-cache-ii-the-unload-event/

How to reproduce with the demo site:

  • disable turbo <html data-turbo="false">
  • open chrome incognito
  • go to https://localhost:8000/
  • click on "Live Components" card
  • write something in the search input
  • navigate back-and-forth
  • try to update the search input

@1ed 1ed force-pushed the fix-live-after-unload branch 4 times, most recently from 22298c5 to cbdc801 Compare August 21, 2022 08:35
@kbond kbond requested a review from weaverryan August 21, 2022 14:10
@weaverryan
Copy link
Member

Thanks for this - nice catch. I believe the original code was added to fix a js error that might happen if our component kept running and tried to operate on elements that no longer appeared on the page after navigation… but I can’t remember correctly, and that commit appears to go back to the “initial commit” commit :/.

So I like your solution, but I’m also not familiar with these events. Also, one post I found says to use the page visibility api - https://www.igvita.com/2015/11/20/dont-lose-user-and-app-state-use-page-visibility/

And, I have one other possible idea. If the original intent was to avoid mutating some elements… when they are no longer on the page, could this be accomplished by connect() and disconnect()? In those methods, we store the state of the component (connected or not) and then skip the rendering if we are not connected?

@1ed
Copy link
Contributor Author

1ed commented Oct 18, 2022

I added a new commit to use connect disconnect, but I don't know how to test this properly, the previous test fails because it used page events. Maybe this something that should be tested by e2e test in a browser? I also added how to reproduce this on the 2.x branch (turbo should be disabled) to the description.

I had an other issue too: 4e846d8#diff-565bf0a4d7c4e904b165c4a2bc70ccb6101d31119b4a88ff0d9ed828537a33dfR3 this file is missing from the repository so the build can not run, or am I missing something?

@1ed 1ed force-pushed the fix-live-after-unload branch 2 times, most recently from 0549236 to 80ad304 Compare October 19, 2022 07:33
@1ed
Copy link
Contributor Author

1ed commented Oct 19, 2022

Tests fixed by triggering controller connect/disconnect.

@1ed 1ed force-pushed the fix-live-after-unload branch from 80ad304 to 089a669 Compare October 19, 2022 07:37
@weaverryan
Copy link
Member

I had an other issue too: 4e846d8#diff-565bf0a4d7c4e904b165c4a2bc70ccb6101d31119b4a88ff0d9ed828537a33dfR3 this file is missing from the repository so the build can not run, or am I missing something?

thanks for mentioning that - it was a bad commit by me. Fixed now.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Wonderful!

@weaverryan weaverryan force-pushed the fix-live-after-unload branch from 089a669 to cc13885 Compare October 19, 2022 14:45
@weaverryan weaverryan merged commit 59babae into symfony:2.x Oct 19, 2022
@1ed 1ed deleted the fix-live-after-unload branch October 20, 2022 07:12
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.

2 participants