-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: issues with observer #80
base: master
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
// Use `intersectionRatio` because of Edge 15's | ||
// lack of support for `isIntersecting`. | ||
// See: https://github.com/w3c/IntersectionObserver/issues/211 | ||
if (entries.some(e => e.isIntersecting || e.intersectionRatio > 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes number of entries in IntersectionObserver
callback is not 1 but 2. Both entries relate to the same element, but one have isIntersected=false
and the other one isIntersected=true
. This causes first intersection to be not captured.
I debugged the code, and observe()
function is called only once, so I don't know how it's possible, that callback has 2 entries.
This I could easily reproduce in newest chrome.
Also, reading through code of vue-lazy-hydrate
I found this line:
https://github.com/maoberlehner/vue-lazy-hydration/blob/master/src/utils.js#L13
So I added the same fix with intersectionRatio
.
src/index.js
Outdated
if (this.observer) { | ||
this.observer.disconnect(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue I could not reproduce, only got error reports through sentry.io. They are I guess specific to some browsers or maybe caused by rat races.
Sometimes this.observer
is null
, so the call fails. Maybe it's when component is destroyed before it was mounted, or maybe it has to do with lazy hydration. Can't tell. But this fix helps.
Thanks! Awesome fix. The PR would need to be adapted to new |
# Conflicts: # v-lazy-image/index-v2.js
Thanks, @kedrzu! I see now it's updated with the latest master 👍 Would you do the fix in the |
@@ -69,7 +71,7 @@ export default { | |||
}); | |||
|
|||
onBeforeUnmount(() => { | |||
if ("IntersectionObserver" in window && state.observer) { | |||
if (state.observer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this condition is needed - if it's not met in onMounted
hook, observer will be null.
Also - no reactive state is dependent on state.observer
, so it can be as well in a simple variable, instead of a reactive object.
In a project I am working on, I encountered some errors with
IntersectionObserver
, so I thought I could fix that.Details in code comments.