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

Update to utilize the PageVisibility API instead of focus event #3

Closed
wants to merge 1 commit into from

Conversation

stramel
Copy link

@stramel stramel commented Feb 12, 2021

Hello 👋, I was looking into @happykit/flags implementation for the revalidateOnFocus I noticed that it utilizes window.addEventListener('focus', ...) for handling the focus. I found a few issues with this implementation:

  • iframe can cause the focus event to fire numerous times if the user interacts with the iframe and then back with the current document
  • DevTools can cause the focus event to fire numerous times if the user/developer interacts with the DevTools and then back with the document. (A very common case for developers)
  • A developer fires a focus event (ie. node.dispatchEvent(new CustomEvent('focus', { bubbles: true}))). This case seems less likely but still a side-effect of the current implementation.

I'm curious if you have considered utilizing the Page Visibility API to achieve a more true implementation for "refocus" of the window.

Thank you for taking the time to consider my suggestion!

@vercel
Copy link

vercel bot commented Feb 12, 2021

@stramel is attempting to deploy a commit to the HappyKit Team on Vercel.

A member of the Team first needs to authorize it.

@dferber90
Copy link
Contributor

dferber90 commented Feb 13, 2021

Wow, that's pretty neat! I wasn't aware of this API.

I looked into swr and it seems like they're doing it that way as well.

I'm currently working on the next iteration of @happykit/flags which will be out in a few days, and which is a complete rewrite. Your suggestion will make it into the rewrite. I don't want to release an update to the current version to avoid having to make people upgrade twice within a few days. So unfortunately I can't merge this PR but the approach will make it in!

In case you're curious, you can check out the rewrite under the next branch. There's an example folder there which is a Next.js application that uses the unpublished new client from the package folder.

Thanks again, seeing your contribution made me really happy :)

@dferber90 dferber90 closed this Feb 13, 2021
dferber90 added a commit that referenced this pull request Feb 13, 2021
@dferber90
Copy link
Contributor

Your suggestion has made it into next now: 30c9038

Thanks again!

dferber90 added a commit that referenced this pull request Apr 12, 2021
@stramel stramel deleted the stramel-patch-1 branch April 12, 2021 04:31
dferber90 added a commit that referenced this pull request May 9, 2021
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