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

polarathene · 2020-07-30T02:15:37Z

Description

Complimentary PR to gatsby-image hydration fix, this larger PR fixes the issue prior to React being available while loading HTML.

If additional details about lines are needed, check individual commits for extra details or logical grouping.

Notes

Potential changes up for discussion

As media queries cannot be inline styles, <style> elements are used within <body>(unless there is an API to add to <head>?), or rather each gatsby-image component instance. These are removed once React hydrates, so only valid in SSR output. Could change that if there's value in keeping around in the client?

The hash could also be used for cache keys instead of img.src if any value in that.

Hash function implementation

The hash method DJB2a which is the same as what styled-components uses should be fine, there isn't any expectation for the results to be compatible with other DJB2a implementations, which might happen if the filepath strings contain chars that exceed a single byte(some languages, emoji, etc), that can be supported if desired but requires more code and adds perf overhead.

Further related details in my original PR comments:

Originally considered/implemented base64 and FNV1a-32 before DJB2a for short CSS classes: fix(gatsby-image): React hydration buster on image #24811 (comment)
DJB2a implementation notes, benchmark tests, etc: fix(gatsby-image): React hydration buster on image #24811 (comment)

`<noscript>` support not added

Old comments

This has not presently catered for <noscript>, which if desired, I'll revert to a previous ResponsiveQueries components in my original PR that used string concatenation with all CSS in a single <style> instead of one <style> per media condition + fallback/base.

There is presently a PR actively discussing if <noscript> logic can be changed to drop the string variants supporting it without bumping the min React to 16.5+, or we can alternatively hold off until a v3 with breaking changes.

Let me know if catering to <noscript> is presently worth the time(it's already partly out of sync IIRC).

EDIT: Shouldn't actually be a problem since this is only relevant for the SSR payload, and targets the root tag of the component.

Not exactly HTML spec compliant, but other popular packages use same approach

While browsers support this approach(and that libraries like Emotion apparently use this technique), <style> elements within body is technically against spec, and thus may be flagged if anyone has some CI / validation checks on HTML spec conformance.

There's not much we can do about this without some SSR API improvements, or adopting some CSS-in-JS library.

Prior comment in original PR about <style> approach considerations: #24811 (comment)

Related Issues

Related: #24748 (comment)

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.

Related: #24811 (Original PR)

polarathene

Some of the changes from this commit, were added from this PR to the main hydration fix PR (relevant commit).

These changes would be relevant for rebasing, but have not been added due to the reliance on isHydrated) which this PR does not presently have any interaction with.

packages/gatsby-image/src/index.js

wardpeet · 2020-09-15T16:12:12Z

Hey @polarathene, Thanks for doing all this work! It's working and the changes are dope but I don't think it's a good idea to accept these changes. Gatsby-image is already super complex and this makes it even more complex, we should let the user create these classes themselves. They can integrate it better in their CSS story (emotion, styled-components, css-modules, ...). Maybe we should just warn people about it in develop.

We never thought art-direction through and now we're here. Sorry for the time you've spent on it.

I don't want to close it just yet, so open for feedback.

`uniqueKey` is a CSS classname friendly base36 string of a 32-bit hash generated from all the image variants `srcSet` values. DJB2a itself is the XOR(`^`) improved version of DJB2, a simple and well known hash function. It has been selected for lightweight footprint and decent performance(minimal overhead), but may suffer slightly compared to FNV1a-32 collision wise. It should be a fairly safe choice to go with, `styled-components` also uses this hash function for a similar purpose. Special care is required for hash functions to work correctly in JS due to it's number type not being a 32-bit native type. Bitwise operators in JS operate in 32-bits however, and `>>>` ensures a positive uint number is returned to match expected hash outputs. Used in future commits for CSS to target, as a short and unique identifier. Only present during SSR, removed once hydrated.

Using the `uniqueKey` CSS class, and `<style>` elements, we can ensure the correct CSS applies for Art Directed images prior to React being ready.

Use the new hash method for `key` prop as well. Original commit for this was lost due to rebase with the other half of the commit was transferred to the hydration fix PR this PR compliments.

Reviewer does not want to merge the feature into `gatsby-image`. Extracted to separate a file as a Higher-Order Component. Reverts changes to `index.js`. Loses key prop improvement with `activeVariant`. May not integrate as smoothly as-is due to changes in hydration fix rendering, may need to keep `uniqueKey` until hydrated. To use, fluid or fixed prop is **expected** to be provided. Import the HOC version `gatsby-image/art-direction` and use as you would `gatsby-image`.

Alternative single `<style>` element with string generated if this commit approach with individual `<style>` element per MQ is a problem: ```jsx return ( <style> {base ? `${base}\n` : ``} {imageVariants .filter(v => v.media) .map(v => `@media ${v.media} { ${variantRule(v)} }`) .join(`\n`)} </style> ) ```

polarathene · 2020-09-17T06:03:45Z

Rebased onto master, extracted the feature out to a separate file as a HOC and removed any changes that could only be used within the gatsby-image component internally. That is explained with notes on using the HOC in this commit message.

You can reject this PR if you like. I updated the PR with an example HOC for the benefit of anyone that wants the feature but is unsure how to implement it. I have not taken time to test these changes. As it's not a class based component with constructor and has not been modified to use hooks, the renders will perform unnecessary overhead re-calculating values that have not changed.

Users on mobile or with fast connections may not experience the issue this PR solves. Might be helpful to link to this PR from the gatsby-image docs Art Direction section with a note about it being required for image dimensions to be correct with Art Direction prior to React being ready.

Fixed with latest commit

To anyone that uses the HOC, note that maxWidth and maxHeight support is missing for fluid image:

gatsby/packages/gatsby-image/src/index.js

Lines 504 to 505 in eaac84b

    
           maxWidth: image.maxWidth ? `${image.maxWidth}px` : null, 
        
           maxHeight: image.maxHeight ? `${image.maxHeight}px` : null,

fluid image is incorrectly handled, paddingBottom style is the first child element of the root tag. There is no classname to target it, and the element tag is ambiguous. > :first-child should work, but may break if the components related internals change in future:

gatsby/packages/gatsby-image/src/index.js

Line 516 in eaac84b

paddingBottom: `${100 / image.aspectRatio}%`,

For fixed images, the bgColor placeholder lacks a class to target:

gatsby/packages/gatsby-image/src/index.js

Lines 630 to 632 in eaac84b

    
           width: image.width, 
        
           opacity: !this.state.imgLoaded ? 1 : 0, 
        
           height: image.height,

You'll need to target it as well as the root tag, so something like .${selectorClass}, .${selectorClass} > :first-child { ${styleOverride} }, at which point the ternary might as well be refactored into a if statement.

As commit message mentions, width and height attributes unrelated to CSS in the HTML may result in some rendering issues (possibly with default object-fit: cover style if dimensions differ from art-directed image until that image request responds to provide actual width/height, inlined base64 should be fine, but critical/eager images may be affected).

Also, it's currently being discussed to use width/height on <source> that will override these img attributes to better support Art Direction, gatsby-image will need an update internally to support that in future if browsers adopt it.

Added support for optional `maxWidth` and `maxHeight` for `fluid` type images. The internal elements (padding for fluid, bgcolor for fixed) are both lacking class names to target specifically but both are the 1st children. Due to review constraints they are being targeted with a pseudo-selector, if the internal structure changes this will of course break. Note that the `fixed` type image has width/height attributes as well that are hard-coded to the first image in SSR build. Not tested if that causes any problems, I think it might affect `object-fit: cover`, but only with `critical` images where these attr values are used until image request responds(or re-validates if cached).

MaxwellKendall · 2020-10-08T14:32:28Z

This needs to go into the gatsby-image documentation. Excellent work!

MaxwellKendall · 2020-10-08T14:33:04Z

@polarathene I can open a PR for the dox to include this as a solution until the next delivery includes this out of the box.

polarathene · 2020-10-09T00:35:13Z

@MaxwellKendall thanks that would be appreciated! 👍

wardpeet · 2020-11-09T09:39:34Z

Closing, this is fixed in our new gatsby-plugin-image component. Thank you for your contribution! <3

polarathene requested a review from a team as a code owner July 30, 2020 02:15

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

polarathene mentioned this pull request Jul 30, 2020

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

Merged

polarathene changed the title ~~feat(gatsby-image) Art Direction SSR fix (compliments hydration fix PR)~~ feat(gatsby-image): Art Direction SSR fix (compliments hydration fix PR) Jul 30, 2020

polarathene requested a review from pvdz July 30, 2020 03:07

This was referenced Jul 30, 2020

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

Closed

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

Closed

This comment has been minimized.

Sign in to view

polarathene added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 30, 2020

This comment has been minimized.

Sign in to view

sidharthachatterjee removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 31, 2020

polarathene mentioned this pull request Aug 14, 2020

Best strategy for checking whether (typeof window !== undefined) while using Hooks #26448

Closed

polarathene commented Sep 12, 2020

View reviewed changes

packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved

packages/gatsby-image/src/index.js Outdated Show resolved Hide resolved

polarathene added 5 commits September 17, 2020 14:27

feat: Embed <style> CSS for correct presentation prior to hydration

2e41b89

Using the `uniqueKey` CSS class, and `<style>` elements, we can ensure the correct CSS applies for Art Directed images prior to React being ready.

chore: Use hash method with key prop

78b5f94

Use the new hash method for `key` prop as well. Original commit for this was lost due to rebase with the other half of the commit was transferred to the hydration fix PR this PR compliments.

polarathene force-pushed the feat/art-direction-embedded-styles branch from e64a026 to e179b56 Compare September 17, 2020 05:12

polarathene force-pushed the feat/art-direction-embedded-styles branch from ee3bd85 to 0c0ae42 Compare September 17, 2020 08:04

wardpeet closed this Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

polarathene commented Jul 30, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

polarathene left a comment

wardpeet commented Sep 15, 2020

polarathene commented Sep 17, 2020 •

edited

Loading

MaxwellKendall commented Oct 8, 2020

MaxwellKendall commented Oct 8, 2020

polarathene commented Oct 9, 2020

wardpeet commented Nov 9, 2020

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

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

Conversation

polarathene commented Jul 30, 2020 • edited Loading

Description

Notes

Potential changes up for discussion

Hash function implementation

<noscript> support not added

Not exactly HTML spec compliant, but other popular packages use same approach

Related Issues

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

polarathene left a comment

Choose a reason for hiding this comment

wardpeet commented Sep 15, 2020

polarathene commented Sep 17, 2020 • edited Loading

MaxwellKendall commented Oct 8, 2020

MaxwellKendall commented Oct 8, 2020

polarathene commented Oct 9, 2020

wardpeet commented Nov 9, 2020

polarathene commented Jul 30, 2020 •

edited

Loading

`<noscript>` support not added

polarathene commented Sep 17, 2020 •

edited

Loading