From a38ccbb9a7414eaac51d61207e046b65bdb860a8 Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Sun, 28 Jun 2020 15:18:23 +0000 Subject: [PATCH 1/8] Do not crash the page if the image component is invoked without parameters --- .../gatsby-image/src/__tests__/__snapshots__/index.js.snap | 2 ++ packages/gatsby-image/src/__tests__/index.js | 6 ++++++ packages/gatsby-image/src/index.js | 4 ++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap b/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap index 5f87b56af2fe4..9e91b76e17ac8 100644 --- a/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap +++ b/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap @@ -49,6 +49,8 @@ exports[` should have a transition-delay of 1sec 1`] = ` `; +exports[` should not not render anything if called without parameters 1`] = `
`; + exports[` should render fixed size images 1`] = `
`, () => { expect(console.warn).toBeCalled() }) + it(`should not not render anything if called without parameters`, () => { + const { container } = render() + + expect(container).toMatchSnapshot() + }) + it(`should select the correct mocked image of fluid variants provided.`, () => { const tripleFluidImageShapeMock = fluidImagesShapeMock.concat({ aspectRatio: 5, diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index 97eea0e5aa8aa..b558a383e9f7c 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -74,9 +74,9 @@ const matchesMedia = ({ media }) => * @return {string} */ const getImageSrcKey = ({ fluid, fixed }) => { - const data = fluid ? getCurrentSrcData(fluid) : getCurrentSrcData(fixed) + const currentData = fluid || fixed - return data.src + return currentData && getCurrentSrcData(currentData).src } /** From 0f9c663418d98d230de59ae0a1746d84ecc7f4b8 Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 30 Jun 2020 11:55:34 +0000 Subject: [PATCH 2/8] added a warning if missing fluid and fixed props --- .../src/__tests__/__snapshots__/index.js.snap | 2 - packages/gatsby-image/src/__tests__/index.js | 8 ++-- packages/gatsby-image/src/index.js | 38 ++++++++++++++----- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap b/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap index 9e91b76e17ac8..5f87b56af2fe4 100644 --- a/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap +++ b/packages/gatsby-image/src/__tests__/__snapshots__/index.js.snap @@ -49,8 +49,6 @@ exports[` should have a transition-delay of 1sec 1`] = `
`; -exports[` should not not render anything if called without parameters 1`] = `
`; - exports[` should render fixed size images 1`] = `
`, () => { expect(console.warn).toBeCalled() }) - it(`should not not render anything if called without parameters`, () => { - const { container } = render() + it(`should warn if missing both fixed and fluid props`, () => { + jest.spyOn(global.console, `warn`) + + render() - expect(container).toMatchSnapshot() + expect(console.warn).toBeCalled() }) it(`should select the correct mocked image of fluid variants provided.`, () => { diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index b558a383e9f7c..228ab7f21ef54 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -71,12 +71,12 @@ const matchesMedia = ({ media }) => * Find the source of an image to use as a key in the image cache. * Use `the first image in either `fixed` or `fluid` * @param {{fluid: {src: string, media?: string}[], fixed: {src: string, media?: string}[]}} args - * @return {string} + * @return {string} or null if not cacheable */ -const getImageSrcKey = ({ fluid, fixed }) => { - const currentData = fluid || fixed +const getImageCacheKey = ({ fluid, fixed }) => { + const srcData = getCurrentSrcData(fluid || fixed || []) - return currentData && getCurrentSrcData(currentData).src + return srcData && srcData.src } /** @@ -109,16 +109,20 @@ const getCurrentSrcData = currentData => { const imageCache = Object.create({}) const inImageCache = props => { const convertedProps = convertProps(props) - // Find src - const src = getImageSrcKey(convertedProps) - return imageCache[src] || false + + const cacheKey = getImageCacheKey(convertedProps) + + return imageCache[cacheKey] || false } const activateCacheForImage = props => { const convertedProps = convertProps(props) - // Find src - const src = getImageSrcKey(convertedProps) - imageCache[src] = true + + const cacheKey = getImageCacheKey(convertedProps) + + if (cacheKey) { + imageCache[cacheKey] = true + } } // Native lazy-loading support: https://addyosmani.com/blog/lazy-loading/ @@ -330,6 +334,10 @@ class Image extends React.Component { constructor(props) { super(props) + if (process.env.NODE_ENV === `production`) { + this.validateProps(props) + } + // If this image has already been loaded before then we can assume it's // already in the browser cache so it's cheap to just show directly. this.seenBefore = isBrowser && inImageCache(props) @@ -360,6 +368,16 @@ class Image extends React.Component { this.handleRef = this.handleRef.bind(this) } + validateProps(props) { + const { fluid, fixed } = props + + if (!fluid && !fixed) { + console.warn( + `gatsby-image expects a 'fixed' or a 'fluid' prop; neither was present.` + ) + } + } + componentDidMount() { if (this.state.isVisible && typeof this.props.onStartLoad === `function`) { this.props.onStartLoad({ wasCached: inImageCache(this.props) }) From 29ebce81553f47bfddf094d392b9716c91f49209 Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 30 Jun 2020 12:18:09 +0000 Subject: [PATCH 3/8] had swapped a condition --- packages/gatsby-image/src/__tests__/index.js | 4 +++- packages/gatsby-image/src/index.js | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/gatsby-image/src/__tests__/index.js b/packages/gatsby-image/src/__tests__/index.js index 701e19525e87d..4df6b14a86584 100644 --- a/packages/gatsby-image/src/__tests__/index.js +++ b/packages/gatsby-image/src/__tests__/index.js @@ -225,7 +225,9 @@ describe(``, () => { render() - expect(console.warn).toBeCalled() + expect(console.warn).toBeCalledWith( + expect.stringContaining(`expects a 'fixed'`) + ) }) it(`should select the correct mocked image of fluid variants provided.`, () => { diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index 228ab7f21ef54..626c208628499 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -334,7 +334,7 @@ class Image extends React.Component { constructor(props) { super(props) - if (process.env.NODE_ENV === `production`) { + if (process.env.NODE_ENV !== `production`) { this.validateProps(props) } From 77dc5451eebd20b093b443e1f979e0cc5aebdf7e Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 30 Jun 2020 12:21:06 +0000 Subject: [PATCH 4/8] doing destructuring in function parameter list --- packages/gatsby-image/src/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index 626c208628499..57d5921e585d0 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -368,9 +368,7 @@ class Image extends React.Component { this.handleRef = this.handleRef.bind(this) } - validateProps(props) { - const { fluid, fixed } = props - + validateProps({ fluid, fixed }) { if (!fluid && !fixed) { console.warn( `gatsby-image expects a 'fixed' or a 'fluid' prop; neither was present.` From d2bf50d9c81de2890f6d91fc95a80dab9b71090f Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 30 Jun 2020 12:35:51 +0000 Subject: [PATCH 5/8] now with functioning tree-shaking --- packages/gatsby-image/src/index.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index 57d5921e585d0..cd3fd2b825976 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -330,12 +330,22 @@ Img.propTypes = { onLoad: PropTypes.func, } +// Intentionally not an instance method because webpack is not able to +// three-shake it away in production +function validateImageProps({ fluid, fixed }) { + if (!fluid && !fixed) { + console.warn( + `gatsby-image expects a 'fixed' or a 'fluid' prop; neither was present.` + ) + } +} + class Image extends React.Component { constructor(props) { super(props) if (process.env.NODE_ENV !== `production`) { - this.validateProps(props) + validateImageProps(props) } // If this image has already been loaded before then we can assume it's @@ -368,14 +378,6 @@ class Image extends React.Component { this.handleRef = this.handleRef.bind(this) } - validateProps({ fluid, fixed }) { - if (!fluid && !fixed) { - console.warn( - `gatsby-image expects a 'fixed' or a 'fluid' prop; neither was present.` - ) - } - } - componentDidMount() { if (this.state.isVisible && typeof this.props.onStartLoad === `function`) { this.props.onStartLoad({ wasCached: inImageCache(this.props) }) From 79670333be618d8b39ae83e1bb669311faf9e28a Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 30 Jun 2020 14:11:26 +0000 Subject: [PATCH 6/8] updating function doc: returns undefined, not null --- packages/gatsby-image/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index cd3fd2b825976..aca129d5b3d9f 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -71,7 +71,7 @@ const matchesMedia = ({ media }) => * Find the source of an image to use as a key in the image cache. * Use `the first image in either `fixed` or `fluid` * @param {{fluid: {src: string, media?: string}[], fixed: {src: string, media?: string}[]}} args - * @return {string} or null if not cacheable + * @return {string} or undefined if not cacheable */ const getImageCacheKey = ({ fluid, fixed }) => { const srcData = getCurrentSrcData(fluid || fixed || []) From d9cc4549ef8adba3eb3a7dc07400c64d126eb2f1 Mon Sep 17 00:00:00 2001 From: Andreas Ehrencrona Date: Tue, 7 Jul 2020 10:57:42 -0300 Subject: [PATCH 7/8] switching to proptypes for validation --- packages/gatsby-image/src/__tests__/index.js | 8 ++-- packages/gatsby-image/src/index.js | 39 ++++++++++++-------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/gatsby-image/src/__tests__/index.js b/packages/gatsby-image/src/__tests__/index.js index 4df6b14a86584..f8892facfc0e2 100644 --- a/packages/gatsby-image/src/__tests__/index.js +++ b/packages/gatsby-image/src/__tests__/index.js @@ -221,12 +221,14 @@ describe(``, () => { }) it(`should warn if missing both fixed and fluid props`, () => { - jest.spyOn(global.console, `warn`) + jest.spyOn(global.console, `error`) render() - expect(console.warn).toBeCalledWith( - expect.stringContaining(`expects a 'fixed'`) + expect(console.error).toBeCalledWith( + expect.stringContaining( + `The prop \`fluid\` or \`fixed\` is marked as required` + ) ) }) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index aca129d5b3d9f..e736cf5d4cca1 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -330,24 +330,10 @@ Img.propTypes = { onLoad: PropTypes.func, } -// Intentionally not an instance method because webpack is not able to -// three-shake it away in production -function validateImageProps({ fluid, fixed }) { - if (!fluid && !fixed) { - console.warn( - `gatsby-image expects a 'fixed' or a 'fluid' prop; neither was present.` - ) - } -} - class Image extends React.Component { constructor(props) { super(props) - if (process.env.NODE_ENV !== `production`) { - validateImageProps(props) - } - // If this image has already been loaded before then we can assume it's // already in the browser cache so it's cheap to just show directly. this.seenBefore = isBrowser && inImageCache(props) @@ -741,6 +727,23 @@ const fluidObject = PropTypes.shape({ maxHeight: PropTypes.number, }) +function requireFixedOrFluid(originalPropTypes) { + return (props, propName, componentName) => { + if (!props.fixed && !props.fluid) { + throw new Error( + `The prop \`fluid\` or \`fixed\` is marked as required in \`${componentName}\`, but their values are both \`undefined\`.` + ) + } + + PropTypes.checkPropTypes( + { [propName]: originalPropTypes }, + props, + `prop`, + componentName + ) + } +} + // If you modify these propTypes, please don't forget to update following files as well: // https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-image/index.d.ts // https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-image/README.md#gatsby-image-props @@ -748,8 +751,12 @@ const fluidObject = PropTypes.shape({ Image.propTypes = { resolutions: fixedObject, sizes: fluidObject, - fixed: PropTypes.oneOfType([fixedObject, PropTypes.arrayOf(fixedObject)]), - fluid: PropTypes.oneOfType([fluidObject, PropTypes.arrayOf(fluidObject)]), + fixed: requireFixedOrFluid( + PropTypes.oneOfType([fixedObject, PropTypes.arrayOf(fixedObject)]) + ), + fluid: requireFixedOrFluid( + PropTypes.oneOfType([fluidObject, PropTypes.arrayOf(fluidObject)]) + ), fadeIn: PropTypes.bool, durationFadeIn: PropTypes.number, title: PropTypes.string, From 3be40bec422f799f284cd9c24a493966eb2d1c57 Mon Sep 17 00:00:00 2001 From: Ward Peeters Date: Tue, 7 Jul 2020 16:27:01 +0200 Subject: [PATCH 8/8] Update packages/gatsby-image/src/index.js --- packages/gatsby-image/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/gatsby-image/src/index.js b/packages/gatsby-image/src/index.js index e736cf5d4cca1..e8842d3d43b46 100644 --- a/packages/gatsby-image/src/index.js +++ b/packages/gatsby-image/src/index.js @@ -71,7 +71,7 @@ const matchesMedia = ({ media }) => * Find the source of an image to use as a key in the image cache. * Use `the first image in either `fixed` or `fluid` * @param {{fluid: {src: string, media?: string}[], fixed: {src: string, media?: string}[]}} args - * @return {string} or undefined if not cacheable + * @return {string?} Returns image src or undefined it not given. */ const getImageCacheKey = ({ fluid, fixed }) => { const srcData = getCurrentSrcData(fluid || fixed || [])