From 9485cf80b040c524b156af4ea3a328e533005a6f Mon Sep 17 00:00:00 2001 From: polarathene <5098581+polarathene@users.noreply.github.com> Date: Wed, 29 Jul 2020 22:44:20 +1200 Subject: [PATCH 1/4] fix: Don't assume DOM state is valid at hydration stage Prevents an initial render from hydration which can mismatch the initial state(React doesn't verify and assumes the DOM represents the same initial state it would compute). This assumption is unreliable when using Art Direction. NOTE: 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. --- packages/gatsby-image/src/index.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index 4c9e43812928b..d77791c71d81e 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -358,6 +358,7 @@ class Image extends React.Component { imgLoaded: false, imgCached: false, fadeIn: !this.seenBefore && props.fadeIn, + isHydrated: false, } this.imageRef = React.createRef() @@ -367,6 +368,10 @@ class Image extends React.Component { } componentDidMount() { + this.setState({ + isHydrated: isBrowser, + }) + if (this.state.isVisible && typeof this.props.onStartLoad === `function`) { this.props.onStartLoad({ wasCached: inImageCache(this.props) }) } @@ -445,6 +450,12 @@ class Image extends React.Component { draggable, } = convertProps(this.props) + // Avoid render logic on client until component mounts (hydration complete) + // Prevents using mismatched initial state from the hydration phase + if (isBrowser && !this.state.isHydrated) { + return null + } + const shouldReveal = this.state.fadeIn === false || this.state.imgLoaded const shouldFadeIn = this.state.fadeIn === true && !this.state.imgCached From 80cd79002c64332c8a43899078fa985bb14b2aac Mon Sep 17 00:00:00 2001 From: polarathene <5098581+polarathene@users.noreply.github.com> Date: Thu, 30 Jul 2020 14:24:22 +1200 Subject: [PATCH 2/4] chore: Fix e2e `gatsby-image` integration tests Not entirely clear why this is necessary, but for some reason the small changes have added white-space to the CSS? --- e2e-tests/gatsby-image/cypress/integration/fluid.js | 2 +- e2e-tests/gatsby-image/cypress/integration/image.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e-tests/gatsby-image/cypress/integration/fluid.js b/e2e-tests/gatsby-image/cypress/integration/fluid.js index 3c88d0c282a90..8ee4f7d87eea0 100644 --- a/e2e-tests/gatsby-image/cypress/integration/fluid.js +++ b/e2e-tests/gatsby-image/cypress/integration/fluid.js @@ -9,7 +9,7 @@ describe(`fluid`, () => { cy.getTestElement(fluidTestId) .find(`.gatsby-image-wrapper > div`) .should(`have.attr`, `style`) - .and(`match`, /width:100%;padding-bottom/) + .and(`match`, /width: 100%; padding-bottom/) }) it(`renders sizes`, () => { diff --git a/e2e-tests/gatsby-image/cypress/integration/image.js b/e2e-tests/gatsby-image/cypress/integration/image.js index 9ef531b4028c5..22d1ca9ddf75d 100644 --- a/e2e-tests/gatsby-image/cypress/integration/image.js +++ b/e2e-tests/gatsby-image/cypress/integration/image.js @@ -14,11 +14,11 @@ describe(`Production gatsby-image`, () => { .should(`eq`, 1) }) - it(`contains position relative`, () => { + it(`has position relative`, () => { cy.getTestElement(fluidTestId) .find(`.gatsby-image-wrapper`) .should(`have.attr`, `style`) - .and(`contains`, `position:relative`) + .and(`match`, /position: relative/) }) }) }) From a464f4e4cd070a338268328b29206741a959e419 Mon Sep 17 00:00:00 2001 From: polarathene <5098581+polarathene@users.noreply.github.com> Date: Sat, 12 Sep 2020 21:39:53 +1200 Subject: [PATCH 3/4] fix: Match SSR state, then fix after hydration As discussed in review, keep initial state in sync with SSR for first render. At hydration `isHydrated` will trigger another render call to update the image to the correct one if different due to art direction usage. Rephrased comments, extracted shared logic from fluid/fixed conditionals (DRY). Doing so fails a test on null image data, so now additionally guard against that early. --- packages/gatsby-image/src/index.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index d77791c71d81e..237db60cb2861 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -450,9 +450,9 @@ class Image extends React.Component { draggable, } = convertProps(this.props) - // Avoid render logic on client until component mounts (hydration complete) - // Prevents using mismatched initial state from the hydration phase - if (isBrowser && !this.state.isHydrated) { + const imageVariants = fluid || fixed + // Abort early if missing image data (#25371) + if (!imageVariants) { return null } @@ -487,10 +487,14 @@ class Image extends React.Component { itemProp, } - if (fluid) { - const imageVariants = fluid - const image = getCurrentSrcData(fluid) + // Initial client render state needs to match SSR until hydration finishes. + // Once hydration completes, render again to update to the correct image. + // `imageVariants` is always an Array type at this point due to `convertProps()` + const image = !this.state.isHydrated + ? imageVariants[0] + : getCurrentSrcData(imageVariants) + if (fluid) { return ( Date: Sat, 12 Sep 2020 23:12:31 +1200 Subject: [PATCH 4/4] chore: Revert e2e test changes Render logic was changed to be similar to how it was prior to this PR, thus these changes are no longer relevant. --- e2e-tests/gatsby-image/cypress/integration/fluid.js | 2 +- e2e-tests/gatsby-image/cypress/integration/image.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e-tests/gatsby-image/cypress/integration/fluid.js b/e2e-tests/gatsby-image/cypress/integration/fluid.js index 8ee4f7d87eea0..3c88d0c282a90 100644 --- a/e2e-tests/gatsby-image/cypress/integration/fluid.js +++ b/e2e-tests/gatsby-image/cypress/integration/fluid.js @@ -9,7 +9,7 @@ describe(`fluid`, () => { cy.getTestElement(fluidTestId) .find(`.gatsby-image-wrapper > div`) .should(`have.attr`, `style`) - .and(`match`, /width: 100%; padding-bottom/) + .and(`match`, /width:100%;padding-bottom/) }) it(`renders sizes`, () => { diff --git a/e2e-tests/gatsby-image/cypress/integration/image.js b/e2e-tests/gatsby-image/cypress/integration/image.js index 22d1ca9ddf75d..9ef531b4028c5 100644 --- a/e2e-tests/gatsby-image/cypress/integration/image.js +++ b/e2e-tests/gatsby-image/cypress/integration/image.js @@ -14,11 +14,11 @@ describe(`Production gatsby-image`, () => { .should(`eq`, 1) }) - it(`has position relative`, () => { + it(`contains position relative`, () => { cy.getTestElement(fluidTestId) .find(`.gatsby-image-wrapper`) .should(`have.attr`, `style`) - .and(`match`, /position: relative/) + .and(`contains`, `position:relative`) }) }) })