-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby-image): check if imageRef is still available #22255
Conversation
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.
lgtm, just curious though, can we use nullish operators here? this.imageRef.current?.currentSrc
We could but we end up with a bigger bundle, that's the problem with new features and babel. Babel adds extra bytes because What's the difference in bytes? not much but in my opinion not worth it. (+14 bytes) nullish: var t;this.setState({imgLoaded:e,imgCached:!!(null===(t=this.imageRef.current)||void 0===t?void 0:t.currentSrc)}) non nullish: this.setState({imgLoaded:e,imgCached:!(!this.imageRef.current||!this.imageRef.current.currentSrc)}) |
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.
Let's get this in - if nothing else some sanity check is never wrong ;)
Trying to understand this PR as I was looking into the
You cite that for lazyloaded components that this can be null causing a crash because they have not been hydrated from the DOM yet...? If hydration is involved, that would imply these aren't lazyloaded but were in the markup during SSR? It should only be run by lazyloaded instances that are using Intersection Observer API as I added You then state it's always been broken and is always false? That was not the case when I created the PR for it, it solved an actual problem. I'd appreciate a test case to reproduce the crash, or clarification for what "lazyloaded" refers to. |
Description
Lazyloaded images don't have an imageRef.current yet because they haven't been hydrated from the dom. We do an extra check to make sure it's not null so we don't crash. imgCached will always stay false but that always has been broken
At least we don't crash