fix(gatsby-image): Don't assume DOM state is valid at hydration stage

[polarathene]

Description

If you use the Art Direction feature with image objects differing in aspect ratios(fluid) or dimensions(fixed), SSR bakes in the CSS for the first image, but the first load(hydration) expects the initial state to match the CSS, this is not always the case if your breakpoint differs from SSR, React doesn't realize it needs to update the CSS.

Notes

An improvement would be to only block the initial render for Art Directed images, although that may burden maintenance due to adding another fork in the render path.

This PR does not resolve the issue prior to React being ready, the wrong state for art directed images will still be an issue there, PR available.

WARNING: This change makes componentDidMount() fire before handleRef(), the isCritical logic will never work as the imageRef will not be available at this point in time. I have a fix for this as a part of larger PR here refactoring the behaviour it is a part of. Original PR related comment (mostly the same information). (Shouldn't apply with September commits)

Related Issues

Fixes #25938
Fixes #24748
Fixes #16888
Fixes #16763

Related: #24811 (Original PR)

[polarathene]

@wardpeet requested e2e tests for the original PR:

is this something we can test in our e2e tests? I would love to have this cause this feels like something that can easily break in refactors

My response:

I'm available on the Discord server(same username) if someone knows how to approach writing the tests and can get me up to speed with e2e testing.

Later, part-two of the PR which provides the CSS fix prior to React loading is a concern for adding an e2e test as it would likely mask this simpler fix. Still open to guidance with e2e if someone is happy to mentor.

[pvdz]

I'm still not sure where the space changes come from, but I'm not too concerned about that.

I also don't remember getting an answer why regex are preferred over a string in the tests. But as they are tests, it's not a big deal to me.

Fwiw, it's okay from me, so I'm leaving it to Ward to accept.

Thank you for splitting this PR, it looks MUCH better for reviewing this way. I will leave this for @wardpeet (who is on vacation currently) to make sure he's okay with this.

Is the description still up to date? In particular, does this slice of the PR still fix all those issues?

[polarathene]

I'm still not sure where the space changes come from

A wild guess is that it's a timing issue of when Cypress takes a look? Could be that SSR CSS lacks the white-space(is there some sort of minification step there?) and after hydration, React setting styles adds those spaces.

As this PR specifically modifies the rendering phase a little bit and forces a render instead of assuming the state matches (which avoids an "unnecessary" render afaik), it could very well be the reason.

I also don't remember getting an answer why regex are preferred over a string in the tests.

Sorry, I must have missed that question. I only changed one test to a regex, I had tried to modify only the white-space in the comparison string, but recall that failed for some reason. The regex didn't and matches the other test, so that seemed fine?

Did you mean testing against the whole CSS string? The each contain some values that are unique to the image data, probably wouldn't hurt doing so, but might add noise to what is actually being tested for?

(who is on vacation currently) to make sure he's okay with this.

Ahhh... was wondering where he disappeared to 😅

Is the description still up to date? In particular, does this slice of the PR still fix all those issues?

Yes, I made sure to confirm that.

[pvdz]

Did you mean testing against the whole CSS string?

No. Just wondering why it had to be .match(regex rather than .includes(string, which also appears to be an existing api. Since this check doesn't use any regex-specific feats, a string would be more efficient and simpler conceptually (and for maintenance). As this is for test cases, perf is less of a concern so it's not very important.

[polarathene]

Is @wardpeet still on vacation? If so could I please know when he's roughly expected back?

I need to know if this PR will be accepted to continue testing/development on my image caching PR.

[wardpeet]

I'll do some testing, my fear is that we get subtle changes because of rendering null. As we get a rehydration mismatch now, we don't show it because of dev vs prod mode.

This doesn't seem to be the right solution, we'll have to find a way to make SSR & CSR work together.

[polarathene]

As we get a rehydration mismatch now, we don't show it because of dev vs prod mode

Can you clarify how to reproduce that console error? Are you saying it's hidden in Gatsby develop and build?

This doesn't seem to be the right solution, we'll have to find a way to make SSR & CSR work together.

The complimentary PR which is much larger adds CSS to the HTML via <style> elements outside of <head> (against HTML spec, but supported by browsers and what popular style libs like Emotion do). AFAIK, that should avoid the state mismatch between SSR and CSR, so this PR may not be required.

I just tested without the forced re-render (returned JSX must differ, hence triggering a render by hydration state bool changing alone does nothing), the problem persists.

The <style> fix does work(without this hydration fix) if the !isBrowser guard is removed from this.uniqueKey in the complimentary PR. Which I guess you'd want anyway since your concern is a difference with the server/client JSX rendered...? (why is that not ever an issue when we conditionally render for the client elsewhere though?)

Although, I personally don't think we should be keeping this CSS fix once hydrated, it uses !important and makes the related inline style code redundant?

[polarathene]

I thought I had followed advice from the official React docs on hydration, but missed the part that it should still match the server on first pass:

React expects that the rendered content is identical between the server and the client. It can patch up differences in text content, but you should treat mismatches as bugs and fix them.

If you intentionally need to render something different on the server and the client, you can do a two-pass rendering. Components that render something different on the client can read a state variable like this.state.isClient, which you can set to true in componentDidMount().

This way the initial render pass will render the same content as the server, avoiding mismatches, but an additional pass will happen synchronously right after hydration. Note that this approach will make your components slower because they have to render twice, so use it with caution.

If we need to return JSX that matches the server before hydration(instead of using null to force revalidation), then perhaps this would be an appropriate solution?:

    const imageVariants = fluid || fixed
    const image = !this.state.isHydrated
      ? imageVariants[0]
      : getCurrentSrcData(imageVariants)

This way we're always using the 1st image available, and only after hydration do we update the image to adjust the inline styles.

[polarathene]

Since no response has been received, I went ahead and committed the change. Hopefully this is in good shape now.

• e2e test changes reverted, no longer required as old render path restored.
• Borrowed some changes that the complimentary PR added, avoids duplicating the hydration fix snippet.

If merging this, do consider #26117 and #26119. They both improve the art direction feature by addressing rendering issues users of the feature care about.

---

I will no longer be available to make any future amendments if needed. The feedback cycle on these PRs(and engagement outside of my own PRs within this repo) is too slow for me (Initial PR opened at the start of June with a fair bit of my time invested into creating these PRs for the benefit of others, I don't use art direction at all).

I understand the core team is quite busy, but the frequency I see interactions effectively abandoned is more common than I'd like. I have been thanked when helping users solve issues raised on Github or community questions/troubleshooting on the Gatsby discord when I was active there. LekoArts has been great, as was pvdz with this original PR and few others I contributed at the time.

I don't benefit from these contributions, a little respect for the time taken to improve gatsby-image for the benefit of Gatsby and it's users would be nice, by responding in a timely manner.. I volunteered time to help improve the experience for members of the Gatsby community, rather than investing that time in myself towards trying to get my first webdev role.

[wardpeet]

I apologize! This is on me, thank you for doing the work and being so patient. Gatsby-image is a difficult component to handle and you're killing it! So many thanks for it. We sadly made the decision to enable art-direction inside gatsby-image. Art-direction is not as easy as it seemed with hydration. However, we'll maintain this until the next breaking change. I'll be merging this one as it fixes the stale state.

Thank you again!

Can you clarify how to reproduce that console error? Are you saying it's hidden in Gatsby develop and build?

This error is only shown when you use react in development mode. Sadly gatsby develop doesn't do SSR. However you can add the following code to gatsby-node.js to mimic the same behaviour:

exports.onPreInit = () => {
  process.env.NODE_ENV = 'development';
}

[pindjur]

@polarathene gatsby-image already uses double rendering when isVisible is set to true. Now double initial render.This is too expensive and here is why. For example, component that fetches images and displaying them in infinite scroll gallery. isVisible is set to true almost immediately, and that is 4 renders for each gatsby-image in short period of time. I think its better to conditionally set state in componentDidMount only if art directed feature is used as you said.

[pindjur]

Updates will be batched because isCritical will be true if we use loading="eager". but still for people that doesn't use eager loading

[polarathene]

isVisible is set to true almost immediately, and that is 4 renders for each gatsby-image in short period of time

Have you done any profiling to measure how much of a difference this PR added for you timing overhead wise?

If you're not using Art Direction, you do not have a use for it. That means the image that gets selected will be the same image. If nothing changes in the returned JSX, there shouldn't be anything costly going on?

but still for people that doesn't use eager loading

isVisible render trigger (Intersection Observer)

Images first have an initial render, only browsers using Intersection Observer (Safari until afaik v14 which is releasing on Sep 16 2020) images have isVisible set as false during this render, unless set to eager(previously critical but merged to use native image loading attribute). When isVisible is false images will hydrate with their placeholder if any, and not have the internal <Img> component mounted until Intersection Observer decides the image is within the viewport (this is much more conservative compared to browsers with native lazy loading).

So for the majority of browsers/users, you'll already have one less state change triggering a new render.

imgLoaded render trigger

Another trigger is boolean state toggle for imgLoaded in handleImageLoaded(). That triggers a render even if you have loaded the image before and it's in your browsers cache. Even critical images will trigger this if they've already loaded before React was ready. I believe it affects images in the gatsby-image internal cache as well, even if they've already loaded, as well as resizing your viewport if it would trigger loading a different size of your image (unrelated to Art Direction). Some of the cases can set imgLoaded to true without waiting on the browser. The thing is, if imgLoaded is already true, setting it to true will still trigger a render, it needs a guard like this:

if (!this.state.imgLoaded) {
  this.setState({ imgLoaded: true })
}

imgLoaded + caching

I investigated into better cache behaviour to reduce renders and undesirable flickers. A workaround that we currently use with Intersection Observer before native image lazy load support arrived in browsers, I updated to use with native lazy load, and some other refactoring. You can see it here, although due to changes at the end of this PR review, I don't know how compatible that PR is anymore with the render path assumptions (eg critical).

Firefox iirc, cannot leverage it, and Safari (possibly non-issue in v14) is getting reports of unexpected behaviour that does not occur on other webkit based browsers with Intersection Observer. Although reports of that issue are still unclear as barely anyone wants to investigate it properly and answer my questions (I do not have access to Safari/macOS to test myself).

---

Render flow

The initial render will run the render method, but it does not update the DOM, it assumes that during hydration phase the returned output will match that of the HTML being mounted. After that, the component will call componentDidMonut() which can trigger another render, or the component will be triggered by Intersection Observer (isVisible to true), or once the image has downloaded (imgLoaded, render reveal transition).

1. Hydration (DOM unaffected, unless React already hydrated and mounting new component during runtime)
2. Component mounted (isHydrated now forces a render)
3. isVisible (Only by Intersection Observer, webkit browsers like Safari or Epiphany)
   4 imgLoaded (Every instance currently)
4. <picture> element <source> changes, eg viewport resized (desktop or orientation change on mobile), triggers imgLoaded again.

So typically 3 renders (+1 for webkit browsers), but actual DOM updates should be 1 for eager otherwise 2(3 with webkit) when no Art Direction, +1 with Art direction.

---

Reducing render trigger by isHydrated

Use global module variable

isHydrated could possibly be global to the module like other parts are, instead of being assigned to the class instance with this. I'm not familiar with React rendering internals to know if componentDidMount() is delayed for all components during hydration, hydrates one components individually, further complicated with async/concurrent rendering I think?

Have Gatsby provide access to know if React has hydrated

There is a way for Gatsby to tell, and perhaps make that available for gatsby-image to know when React has hydrated.

Only setting isHydrated for Art Direction

Alternatively, we could only set isHydrated to true when using Art Direction, this PR could modify the constructor to set this.isArtDirected = hasArtDirectionSupport(imageVariants) instead of the conditional statement on hasArtDirectionSupport(). componentDidMount could then reference that as a guard condition for setting isHydrated. That linked PR also implements the imgLoaded guard as well (although it's mostly only benefiting Art Directed images IIRC).

Assuming nothing but art direction within the component would ever cause a hydration mismatch, that'd be fine.

[polarathene]

@pindjur I've updated this PR to only modify isHydrated when using Art Direction. That does make isHydrated a bit of misleading name at a glance but shouldn't cause any problems.

For that PR to be merged, someone will need to finish it off(needs to handle removing listeners on unmount) and merge it. @wardpeet has agreed to that, but he is also looking into a rewrite of gatsby-image I think, so my PRs may become irrelevant.