fix(gatsby-image): add required check for one of fluid or fixed

[ehrencrona]

Description

Prevents the rendering from crashing if Gatsby image is called without any parameters. The error message that was printed before this change was not helpful to users.

Related Issues

Fixes #25370

[polarathene]

Thanks for taking the time to put together this PR!

Averting the failure is nice, but without a warning/error this could be trading one bug for another with a silent failure for anyone who does use the component without providing a fluid or fixed object.

I would prefer a reason for why a developer would choose to use gatsby-image without providing it that object intentionally? Doesn't a silent failure make it more difficult to track down vs a runtime error(albeit not a very useful one as you point out).

It also looks like the error is actually from the called method attempting to access 0 as an object key rather than index, not the subsequent src property?

[LekoArts]

Coming from #24855 - can you please add a test case where you pass in null to the fluid/fixed props?

[ehrencrona]

Wow, that triggered a lively debate! :D Thanks for all the feedback.

I've updated the PR with a warning. The warning only appears in development; I've checked that the code gets tree-shaken out in production builds.

I also applied the feedback from @polarathene about making sure the value is not cached.

Given #24855 it seems like this is something that can appear at runtime so we shouldn't bomb the page if it happens. The test ought to guarantee that.

Using null or undefined (=omitting the prop) in the test indeed ought to be equivalent. I switched to null since it's more explicit.

A try ... catch is problematic in a class component: if the constructor bails out but doesn't throw, the subsequent methods will fail. If we want to protect the page, an error boundary sounds better to me.

I think it's possible to trigger this via handleImageLoaded():

Inside render we have two blocks: if (fluid) and if (fixed). If neither, no image is added and no loaded event fired, so that should be safe and correct.

(It would, by the way, be nice to pull those two if blocks out into their own functions to make the rendering logic more readable, but I didn't want to include that in this PR)

[polarathene]

(It would, by the way, be nice to pull those two if blocks out into their own functions to make the rendering logic more readable, but I didn't want to include that in this PR)

There's a fair amount of refactoring that needs to be done on gatsby-image. Some to better prep for v3, and then moving from a class component to hooks based for v3. No need to refactor those functions out of render, there's a number of PRs for this component waiting on being reviewed/merged, the less conflicts introduced the better :)

[polarathene]

LGTM, thanks for putting this together! 😀

Good idea with the src renames to more appropriate names.

[wardpeet]

Hey @ehrencrona,

Thanks for creating this PR. Could we move this validation to propTypes?

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
// https://github.com/gatsbyjs/gatsby/blob/master/docs/docs/gatsby-image.md#gatsby-image-props
Image.propTypes = {
  resolutions: fixedObject,
  sizes: 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,
  alt: PropTypes.string,
  className: PropTypes.oneOfType([PropTypes.string, PropTypes.object]), // Support Glamor's css prop.
  critical: PropTypes.bool,
  crossOrigin: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]),
  style: PropTypes.object,
  imgStyle: PropTypes.object,
  placeholderStyle: PropTypes.object,
  placeholderClassName: PropTypes.string,
  backgroundColor: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]),
  onLoad: PropTypes.func,
  onError: PropTypes.func,
  onStartLoad: PropTypes.func,
  Tag: PropTypes.string,
  itemProp: PropTypes.string,
  loading: PropTypes.oneOf([`auto`, `lazy`, `eager`]),
  draggable: PropTypes.bool,
}

[ehrencrona]

@wardpeet that would make sense of course; i can add that. but i'd say it should be in addition to the check in the PR, not a replacement for it, as the issue can also appear at runtime by a variable passed as fluid or fixed becoming null.

[wardpeet]

Why wouldn't this be a replacement for it? I'm only talking about the validation part. The other changes you made are solid 💯.

[ehrencrona]

@wardpeet Oh, I thought proptypes were only evaluated at compile time. Ok, no, if they're evaluated at runtime it's a strictly better solution of course. I'll update the code.

[polarathene]

The propTypes validation is for development only and will work at run-time afaik, when built for production like the validation check code being wrapped in a conditional to exclude it, the propTypes aren't present either.

[ehrencrona]

Ok, I've updated the code. Seems to work well.

[wardpeet]

Sweet! Thank you for taking the time to fix your review and for improving gatsby-image @ehrencrona.

[polarathene]

@wardpeet Please merge? :)

[polarathene]

@wardpeet Are the approved changes going to be merged? (EDIT: Oh... waiting on a @gatsbyjs/content all this time..)

[nicktouchette]

Should the required check extend to sizes as well?

Using something like
<Img sizes={{ ...data.banner.childImageSharp.fluid, aspectRatio: 21 / 9 }} />

Will give this warning
Warning: Failed prop type: The prop `fluid` or `fixed` is marked as required in `Image`, but their values are both `undefined`.

[polarathene]

gatsby-image isn't equivalent to <img> element. It does not have a supported sizes prop, that's handled internally based on fluid or fixed prop being used.

There is the old resolutions and sizes props still technically supported, which last I knew were meant to state they're deprecated, doesn't seem like the component is logging that warning like it does for the critical prop. Seems it was discussed in my old PR, but didn't get a reply from @wardpeet where he resolved it unanswered instead.

Users should be using fixed or fluid prop types by now. Is there any reason that you're not?

[nicktouchette]

@polarathene The reason I'm not using fluid is because I needed to enforce an aspect ratio of 1 on a batch of fluid images that I need to remain perfectly square.

The code piece I referenced comes directly from https://www.gatsbyjs.org/tutorial/gatsby-image-tutorial/ .

If deprecated, what would be the preferred method to enforcing an aspect ratio on fluid images?

[polarathene]

@nicktouchette thanks for that link, I'll create a PR to update that, along with adding the deprecated notice for sizes and resolutions.

If deprecated, what would be the preferred method to enforcing an aspect ratio on fluid images?

sizes works the same as fluid, so changing the prop alone should work.

[nicktouchette]

@polarathene Nice! I changed the prop back to fluid and everything's good. Thanks for the insight.