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

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

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

wardpeet
Copy link
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 as a code owner March 13, 2020 18:04
blainekasten
blainekasten previously approved these changes Mar 14, 2020
Copy link
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
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
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
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.

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