Skip to content

fix(gatsby-image): check if imageRef is still available#22255

Merged
gatsbybot merged 4 commits intogatsbyjs:masterfrom
wardpeet:fix/gatsby-image-null
Mar 20, 2020
Merged

fix(gatsby-image): check if imageRef is still available#22255
gatsbybot merged 4 commits intogatsbyjs:masterfrom
wardpeet:fix/gatsby-image-null

Conversation

@wardpeet
Copy link
Copy Markdown
Contributor

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

@wardpeet wardpeet requested a review from a team March 13, 2020 18:04
blainekasten
blainekasten previously approved these changes Mar 14, 2020
Copy link
Copy Markdown
Contributor

@blainekasten blainekasten left a 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

@wardpeet
Copy link
Copy Markdown
Contributor Author

wardpeet commented Mar 16, 2020

We could but we end up with a bigger bundle, that's the problem with new features and babel. Babel adds extra bytes because nullish operators as it checks null & undefined. I can shortcut this by just testing truthy.

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)})

@wardpeet wardpeet closed this Mar 16, 2020
@wardpeet wardpeet reopened this Mar 16, 2020
Copy link
Copy Markdown
Contributor

@pieh pieh left a 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 ;)

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 20, 2020
@gatsbybot gatsbybot merged commit 363279a into gatsbyjs:master Mar 20, 2020
@wardpeet wardpeet deleted the fix/gatsby-image-null branch March 20, 2020 13:05
@polarathene
Copy link
Copy Markdown
Contributor

Trying to understand this PR as I was looking into the TODO comment it adds. A little further up my comment explains why there is a nested setState call here due to imageRef.current not being available until isVisible is true which mounts the img element:

Once isVisible is true, imageRef becomes accessible, which imgCached needs access to.

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 imgCached before native lazy loading support arrived.

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.

raffishquartan pushed a commit to raffishquartan/gatsby that referenced this pull request Apr 28, 2026
* fix(gatsby-image): check if imageRef is still available

* Update index.js

Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot: merge on green Gatsbot will merge these PRs automatically when all tests passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants