fix(gatsby-image): React hydration buster on `image` #24811

polarathene · 2020-06-05T12:54:13Z

Description

If you use art-direction with differing 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.

The PR provides addresses this by setting image variable to {} for initial state(first frame), so that React will update image state correctly with the next rendered frame(triggered when the component mounts).

Related Issues

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

Extra details

Original issue

SSR and Client states differ for what image is used which can change the aspectRatio affecting the fluid image component bottomPadding style, causing incorrect rendering.

`isBrowser && !isHydrated` to bust the hydration state

This introduces a new bool state value as advised for handling this situation by the official React docs on hydration. I combine it with isBrowser to temporarily ~~adjust the aspectRatio state a minimal amount~~ set an empty initial image state before the next render(triggered upon component mount once hydrated).

Unable to resolve for slow connections prior to React being loaded

That would require embedding JS during SSR to run ASAP on the client, or injecting style tags into the HTML head covering each images breakpoints(I know how to do this with styled-components, but not without it.

Marked some discussion below as "outdated" to hide it, it's all addressed in visible content, should help minimize time needed to review.

Main considerations for reviewer:

Is this an acceptable fix to the hydration issue?
Should <style> be added to the component with opt-out prop? (handles drawbacks, while the main hydration fix will still work)

polarathene · 2020-06-15T00:21:50Z

As discussed below, this should probably be addressed at SSR by adding <style> in body(emotion does this, despite being against spec), we could make it opt-out for anyone if it poses an issue?

`<style>` in DOM body considerations

For fixing the CSS before react has loaded, I have been suggested to use the `<style>` element within the HTML body, instead of the documents head. However, while that is supported by most browsers AFAIK, [it is against spec](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/style), so HTML validators will flag such usage, which may be undesirable for a core gatsby package?

There is advice like this around on the internet from a while ago that suggests it's ok because of the scoped attribute being a future specification browsers would support, but as MDN points out that is presently deprecated due to issues mentioned in their linked issue.

If we ignore this and just add <style> element into the component markup, this should solve pre-React phase of loading a page with this issue, but using style elements out of the document head is said to potentially cause FOUC(flash of unstyled content) / re-paint / re-layout.

circlingthesun · 2020-06-26T14:36:49Z

I mistakenly posted this on the issue instead of the PR:

While this kinda works, it results in a flash of the wrong image until the JS kicks in. If the paddingBottom was determined via media queries instead that would eliminate the issue.

wardpeet · 2020-07-03T07:59:06Z

@polarathene 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

polarathene · 2020-07-03T10:58:23Z

I have no e2e test experience, and I'm not sure if anyone on the gatsby team has the spare time to provide personal guidance on that? Is the window object available for such tests? Or is it like SSR during builds with a limited environment?

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. I am not sure how much of a time commitment that would be for me to figure out on my own.

~~I'm waiting on feedback about <style> in body. Do you want that added by default, with opt-out for those it is a problem for?~~

polarathene · 2020-07-07T01:21:14Z

<style> support added, is only used during SSR for art directed images.

Expand for more details of implementation

I've added support for <style> embeds within the component. This is only used during SSR and only if Art Direction feature is in use. For correct targeting as the styles are global scope, I've added a unique class name to the wrapper, which is derived from an ~~FNV-1a~~ DJB2-XOR hash of the combined srcSet values across fluid/fixed data.

Outdated notes on previous implementation (with benchmark)

I previously converted the number to a base64 string, but noticed the performance and additional code complexity wasn't worth it vs base36. You can see a comparison benchmark(including against JSON.stringify()) here.JSON.stringify() there is about ~40% faster than the hash+base36 logic, but it's really small computation time(~0.1ms).

An alternative hash would be djb2 which styled-components uses for example, slightly more chance of a collision, but still highly unlikely to be an issue.

polarathene · 2020-07-07T04:18:14Z

Using DJB2-XOR for hashing now, it performs well, styled-components uses the same core algorithm, but converts to string differently.

Details of change to DJB2 hash, includes benchmarks

I have looked into DJB2 with the following bench test against short and long strings with some variations. A newer variant over the original uses XOR(^) operator which gives a notable perf boost, this is what styled-components switched to early this year for v5. Here I compare against FNV-1a and JSON.stringify(), there is a notable 2x gain over JSON.stringify() with the XOR variant, ~~while the original DJB2 is on par with FNV-1a.~~(original DJB2 is flawed in JS land). Note that the ~200% performance displayed here drops down to ~120% after .toString(36) is involved.

Local naive test with console.time("label")+console.timeEnd("label") and noted the following example on gatsby build:

JSON.stringify( image.srcSet ): 0.077ms
getShortKey( image.srcSet ): 0.067ms
getShortKey( imageVariants.map(x => x.srcSet).join() ): 0.051ms

Same results in Chrome however, JSON is much faster, less than 0.01ms.

Reducing overhead from conversion back to string

The following would be a way to improve performance if the extra code is worth it(I doubt it):

Initialize with a LUT for mapping a 10-bit index to two characters at a time:

// Takes the base32(5-bit) charset, equivalent to toString(32)
// Creates a Lookup Table of two pairs(2^10, 10-bit == 1024)
const charset = "0123456789abcdefghijklmnopqrstuv".split('')
const LUT = charset.map(x => charset.map(y => x+y)).flat()

Direct lookup seems to have minimal perf overhead vs toString(32)(takes ~40% of benchmark time). 0x3ff is 1023(10-bits) used to isolate the shifted 10-bits, providing the index value:

const n = djb2_xor(long_str)
const mask = 0x3ff

return LUT[ (n >> 30) & mask ] + 
  LUT[ (n >> 20) & mask ] +
  LUT[ (n >> 10) & mask ] +
  LUT[ n & mask ]

Drawback with the LUT is for this 4 lookup approach, values will be zero padded from the left, unlike toString(32) which trims those. If you want to remove those with .replace(/0*/,'') it's fairly costly taking up about ~30%` of time in benchmark testing.

A slightly more performant toString(32) of the above that's a bit easier to grok, but performs at ~73% (removing the replace only brings it to ~79%):

const shifts = [30,20,10,0]
const n = djb2_xor(long_str)

shifts.map(m => LUT[n >> m & 0x3ff])
  .join('')
  .replace(/0*/,'')

// or
let hash = ""
shifts.forEach(m => { hash += LUT[ (n >> m) & 0x3ff ] })
hash.replace(/0*/,'')

Technical explanation of DJB2 algorithm choices / comparison

I investigated styled-components implementation. For anyone curious, their phash doesn't return with >>> 0 because this is a progressive/rolling version where they feed the returned hash into the method again with the next string contents, apparently to make it more unique and avoid hash collisions from their inputs. Not likely to be a concern for how it's being used in gatsby-image here.

I also did some tests on other variants, original 33 * h + character, and bitshift mul ((h << 5) +h) + character. Original can end up with Infinity(.toString(36) therefore casts to "0"), and the bitshift mul seemed to avoid that issue, but still ran into consistency issues(long_str in the linked benches produces different results if not cast to unit(>>> 0), likely due to the addition following the bitshift mul. For the XOR variant, JS will operate on the number as 32-bit with bitwise operators, the number therefore never exceeds the 32-bit range, but is still an int which can be negative, so to avoid the - prefix being added, casting to uint is still required.

Thus XOR is the way to go. while vs for loops didn't seem to matter performance wise(historically they did afaik), so that difference is negligible.

The other notable rendering issue from hydration is on `fixed` images with their width/height values, there are other image properties that get referenced and may pose a hydration issue, I've opted for a blanket empty object initial state instead for `image`, and moved the variable out of the conditional.

Inversing the condition logic so it can avoid rendering the component until mounted(hydrated). Previous logic worked, but would render the `fluid`/`fixed` markup and CSS, despite referencing an empty `image` object, notably for the aspect ratio calculation (`padding-bottom` style), this resulted in an invalid value, causing e2e tests to fail. The initial render of the component prior to mount will assume served HTML/CSS styles can be trusted as matching state for hydration, this isn't always the case so enforce a re-render upon mount.

While not compliant to HTML spec as `<style>` should only be used within `<head>` in the DOM, this approach is used elsewhere such as with Emotion. This fixes the the issue with appropriate CSS for art direction prior to React being available on the client. As each component instance needs to be individually targeted(the `<style>` is not scoped otherwise), a classname unique to the image data is required. This has been handled via a simple and fast hashing algorithm which shouldn't have any collision issues. `uniqueKey` generates a 6 character long(roughly base64) class name, and does so faster across multiple `srcSet`s than the previous `JSON.stringify()` approach did on a single `srcSet`. An alternative simple hashing algorithm, djb2 could be used. This is what styled-components uses as it's faster, but may result in a slightly higher risk of collisions.

Split hash method and post-process of hash into separate functions. Added some helpful comments to explain why the final value is modified. The use of bit shifts with addition instead of multiplying against the prime appears to perform slightly worse in JS, so keep it simple and more readable instead. For reference: https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function#FNV_hash_parameters http://www.isthe.com/chongo/tech/comp/fnv/ > 32 bit FNV_prime = 2^24 + 2^8 + 0x93 = 16777619

Also avoid generation of `<style />` if only one image available (no art direction used).

Refactors the condition out of the `ResponsiveQueries` component. The style component and class name should additionally only be used for SSR and can be discarded from the client side unless we later have use for it.

With the SSR only render, this feature is no longer detected by Jest when creating snapshots.

The while loop was also iterating the wrong way, whoops. This algorithm works alright with 32-bit integers, and overflows, but JS uses 53-bit floats which leads to inaccurate results. One way to avoid the issue is like so: ``` // Split into two 16-bit values, multiply then combine to a 32-bit value h = (((h >> 16) * fnvPrime) << 16) + ((h & 0xffff) * fnvPrime) ``` However that has worse performance and readability vs the bit shift equivalent of multiplication on the prime. Finally, the returned value also needs to be cast to 32-bit uint. `fnv1a32("hello").toString(16)` now correctly returns `4f9f2cab`.

base64 code and handling can be dropped for base36 instead, natively supported in JS `.toString(radix)`. Numbers are part of the charset, so the underscore prefix is still relevant. Only downside is slightly longer class name, from average 5.33 bits to 6.4 bits, values are now 6-7 in length(+ prefix).

DJB2 performs similar to FNV-1a-32, but the XOR(`^`) variant has a perf boost, `styled-components` adopted this variant for v5 early in 2020.

Instead of guarding the fluid/fixed render logic, just guard at the start of render and return null early. Avoids unnecessary indent and execution of logic that won't be used. Also changes unique key to a const by replacing the if statement for a short-circuit assignment. Potentially failed tests due to looking up array length when tests might have been on a single object where length would be an invalid property.

Not sure how this PR is affecting these, but looks like a whitespace issue is failing the tests..

Inlined logic and split each style into their own `<style>` tag, moving the query to the `media` attribute.

Move `uniqueKey` into constructor. It is intended for SSR, so it should be defined under the assumption a `fluid` or `fixed` object is provided to use it during build. Avoids recomputing pointlessly at render time. Renamed `hasArtDirectionSupport()` to `isUsingArtDirection()` which is clearer in intent.

Previously, the browser would update the `<picture>` element image to match a new media condition. Once the request responded with an image to download, the `onload` event triggered and reports `ref.complete` from the `img` element as true. That then triggers `handleImageLoaded()` which would always set the `imgLoaded` state to true, even it already was. That triggers React to re-render which then notices the `img` src has changed and adapts to the correct image variant data for rendering. The re-render was delayed until a request from the server responds with the new image to begin downloading. This is a notable transition period on slow connections such as 3G. There's no placeholder once the re-render occurs, instead, the image will progressively load. The `handleImageLoaded()` event and state update will happen again on completion triggering one more render, as the re-render modified the DOM, resetting the `onload` state(which AFAIK, isn't specific to any `img` element, but the DOM as a whole). This change listens for the media queries via JS API, and promptly re-renders as soon as it actually should. The cache is properly checked for the variant to know if the transition from placeholder should occur or be skipped, just like a regular gatsby-image instance with no art direction involved. As we're triggering the re-render sooner by setting `imgLoaded` to `false`, we don't have to rely on `imgLoaded` state (with no change from `true`) in `handleImageLoaded()` to get the ball rolling with a re-render now. Instead we can ensure that it only sets `imgLoaded` to `true` as it's intended for, only when the state is `false`.

Adds a package to provide better mocking functionality, no longer raises error about missing methods. Improves the existing usage in `gatsby-image` tests.

Leave `fadeIn` logic to `render()` method, otherwise as soon as it's set to false, it will always be false.

polarathene · 2020-07-29T09:22:21Z

I'm going to split the PRs out, should be easy enough to review the basic hydration fix, then consider the <style> embed as a complimentary addition to merge afterwards, related matchMedia() fix will also be split out into it's own PR as requested.

pvdz · 2020-07-29T10:07:40Z

Thanks for your detailed response.

In the end, PRs get squashed and merged so individual commits are mostly useful for review time. But when a PR contains 22 individual commits I tend to skip the individual commits in favor of the whole PR because it's just more efficient. Otherwise often you'd be reviewing a commit and then read a followup commit undoing that because reasons. I'm very sorry if you did happen to have 22 clean commits. I wish there were more of you.

If you prefer I can split it out to it's own PR?

Yes, looking at the PR on a whole, I think that should be split out.

They're required for the feature described above, ...

Unfortunately github doesn't really do "stacks". So your suggestion to have a chain of PRs might be best. I think two is fine here? One with the mock change stuff and then one followup PR with the concrete changes?

It's not checking if support exists, it's checking if this type of support is to be used due to providing an array of image data.

It literally checks if the currentData has any image with a .media property that is not undefined. Ugh, too much bikeshedding, whatever. If you feel strong about it then keep it.

Do you want to keep <style> embed with the hydration fix?

I think you created good splits. Would be great if you can create them as PRs.

due to how long these take to get reviewed/merged

Sorry about that. The team isn't that large and we have a lot on our plate. I merged the PR that you mentioned, sometimes things just slide off our radar. That and holiday season ... Oh well. Request a review from me in these split PR's and I'll try my best to keep up.

Thank you! 💜

polarathene · 2020-07-29T10:22:19Z

I'm very sorry if you did happen to have 22 clean commits. I wish there were more of you.

They're reasonably documented/focused commits, but some are outdated after original submission. I will split the PR apart into clean commits and hopefully we can get them merged with less delay :)

Sorry about that. The team isn't that large and we have a lot on our plate.

I'm available for any paid part-time/full-time remote work if that'd help 😅

I merged the PR that you mentioned, sometimes things just slide off our radar.

Thanks! I understand that how that is, all good. I created a tracking issue, mostly for myself for the image related PRs and will continue to chase those up.

Request a review from me in these split PR's and I'll try my best to keep up.

Will do! Thanks for taking the time to review and merge these, it's very much appreciated.

polarathene · 2020-07-29T13:08:06Z

@pvdz PR split to the following:

Closing this PR.

polarathene requested a review from a team as a code owner June 5, 2020 12:54

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 5, 2020

freiksenet assigned madalynrose Jun 5, 2020

This comment has been minimized.

Sign in to view

polarathene changed the title ~~fix(gatsby-image): React hydration buster on image aspectRatio~~ fix(gatsby-image): React hydration buster on image Jun 6, 2020

This comment has been minimized.

Sign in to view

polarathene requested a review from wardpeet June 13, 2020 04:19

LekoArts mentioned this pull request Jun 16, 2020

[gatsby-image] mobile image is cropped and have desktop image size #25004

Closed

LekoArts assigned wardpeet and unassigned madalynrose Jun 16, 2020

This comment has been minimized.

Sign in to view

polarathene mentioned this pull request Jul 3, 2020

[Tracking]: gatsby-image PRs waiting on review and merge #25487

Closed

7 tasks

This comment has been minimized.

Sign in to view

polarathene force-pushed the fix/gatsby-image-media-aspect-ratio-init branch 3 times, most recently from 9bcaf37 to 1d2e444 Compare July 13, 2020 01:13

polarathene and others added 21 commits July 29, 2020 14:51

chore: Appease the CI

7c1c432

chore: Fix incorrect prop for <ResponsiveQueries />

f839c30

Also avoid generation of `<style />` if only one image available (no art direction used).

chore: Update gatsby-image snapshot

5a922b3

chore: Conditionally create uniqueKey classname

ada3420

Refactors the condition out of the `ResponsiveQueries` component. The style component and class name should additionally only be used for SSR and can be discarded from the client side unless we later have use for it.

chore: Revert gatsby-image snapshot

1f97459

With the SSR only render, this feature is no longer detected by Jest when creating snapshots.

feat: Use DJB2-xor instead of FNV-1a-32

9270c41

DJB2 performs similar to FNV-1a-32, but the XOR(`^`) variant has a perf boost, `styled-components` adopted this variant for v5 early in 2020.

chore: Minor changes

80e3e85

fiix: e2e gatsby-image tests

dd05e60

Not sure how this PR is affecting these, but looks like a whitespace issue is failing the tests..

chore : Refactor ResponsiveQueries component

becc8bc

Inlined logic and split each style into their own `<style>` tag, moving the query to the `media` attribute.

fix: Jest tests better matchMedia() mock

d62bdb8

Adds a package to provide better mocking functionality, no longer raises error about missing methods. Improves the existing usage in `gatsby-image` tests.

chore: Minor change to re-run CI

ebcb8f8

chore: handleVariantChange() should set state based on seenBefore

f39122a

Leave `fadeIn` logic to `render()` method, otherwise as soon as it's set to false, it will always be false.

polarathene force-pushed the fix/gatsby-image-media-aspect-ratio-init branch from 84b812d to f39122a Compare July 29, 2020 09:16

polarathene mentioned this pull request Jul 29, 2020

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

Merged

This was referenced Jul 30, 2020

feat(gatsby-image): Art Direction SSR fix (compliments hydration fix PR) #26117

Closed

feat(gatsby-image): Responsive Art Direction #26119

Closed

polarathene closed this Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gatsby-image): React hydration buster on `image` #24811

fix(gatsby-image): React hydration buster on `image` #24811

polarathene commented Jun 5, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

polarathene commented Jun 15, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

circlingthesun commented Jun 26, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wardpeet commented Jul 3, 2020

polarathene commented Jul 3, 2020 •

edited

Loading

This comment has been minimized.

polarathene commented Jul 7, 2020 •

edited

Loading

polarathene commented Jul 7, 2020 •

edited

Loading

polarathene commented Jul 29, 2020

pvdz commented Jul 29, 2020 •

edited

Loading

polarathene commented Jul 29, 2020

polarathene commented Jul 29, 2020 •

edited

Loading

fix(gatsby-image): React hydration buster on image #24811

fix(gatsby-image): React hydration buster on image #24811

Conversation

polarathene commented Jun 5, 2020 • edited Loading

Description

Related Issues

Original issue

isBrowser && !isHydrated to bust the hydration state

Unable to resolve for slow connections prior to React being loaded

This comment has been minimized.

This comment has been minimized.

polarathene commented Jun 15, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

circlingthesun commented Jun 26, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wardpeet commented Jul 3, 2020

polarathene commented Jul 3, 2020 • edited Loading

This comment has been minimized.

polarathene commented Jul 7, 2020 • edited Loading

polarathene commented Jul 7, 2020 • edited Loading

polarathene commented Jul 29, 2020

pvdz commented Jul 29, 2020 • edited Loading

polarathene commented Jul 29, 2020

polarathene commented Jul 29, 2020 • edited Loading

fix(gatsby-image): React hydration buster on `image` #24811

fix(gatsby-image): React hydration buster on `image` #24811

polarathene commented Jun 5, 2020 •

edited

Loading

`isBrowser && !isHydrated` to bust the hydration state

polarathene commented Jun 15, 2020 •

edited

Loading

polarathene commented Jul 3, 2020 •

edited

Loading

polarathene commented Jul 7, 2020 •

edited

Loading

polarathene commented Jul 7, 2020 •

edited

Loading

pvdz commented Jul 29, 2020 •

edited

Loading

polarathene commented Jul 29, 2020 •

edited

Loading