Allow custom props to Gatsby-Image component

[pavsidhu]

Summary

Custom props should be allowed to be passed to the root tag of a Gatsby Image component.

Basic example

import Image from 'gatsby-image'

<Image customProp={1} />
<Image {...customProps} />

In this example, customProp and customProps should be passed to the root element of the Image component.

Motivation

I'm sure there are several use cases where custom props would be useful. A good example is for flip animations. React-flip-toolkit, requires custom props to be passed to the root element of a React component. Without the ability to pass custom props, flip animations are not possible.

[marcysutton]

I think this would be useful for ARIA attributes, even though Gatsby Image adds an empty alt="" attribute there may still be times that developers want to add something else, like role="presentation". That isn't currently possible.

[ooloth]

Would also be useful for data attributes (data-testid, etc).

[wardpeet]

The question I have here why would you want to augment the image itself and not add a wrapper around it? We'll be making the gatsby-image more modular in the future to make this use case possible. As of know I rather do not want to add this feature as it adds extra complexity without a benefit.

@marcysutton if I add role="presentation" to the parent wouldn't the whole item be marked as presentation? Wouldn't that do the same?

[ooloth]

@wardpeet An extra wrapper is one way to go, that's true.

One side effect of requiring an additional wrapper around the already wrapper-heavy gatsby-image output would be having to updating your CSS (i.e. layout styles that target .gatsby-image-wrapper) anytime you need to pass an attribute to the root element (e.g. adding a data- attribute as a hook to test conditional rendering logic).

So, one benefit of passing valid HTML attributes through to the root DOM element would be avoiding that extra, unrelated work.

[polarathene]

Responding here as per wardpeets request on the associated PR.

I'm not entirely satisfied with this, tagProps={{ role: "img", "aria-label": "Description" }} is not the nicest way to add accessibility, but I understand the problems raised with the complexity of the component.

@RetroCraft this is how passing of props is being supported internally for <Placeholder />:

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

Lines 265 to 270 in b8a355d

	const Placeholder = React.forwardRef((props, ref) => {
	const { src, imageVariants, generateSources, spreadProps, ariaHidden } = props
	const baseImage = (
	<Img ref={ref} src={src} {...spreadProps} ariaHidden={ariaHidden} />
	)

If you were to use a dummy component, you can specify props in a more natural way if you prefer. You can then acquire the props object to pass those through, or if we accepted the component as a prop itself, we could do the same internally, albeit I'm not fond of that.

const data = (<div testProp="hello world" />)
console.log(data.props)

That said, the spread destructuring has been used internally since the inception of the component, and it probably should avoid that too..

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

Line 294 in b8a355d

...otherProps

---

I'm inclined to agree with @wardpeet that it might be best to hold off until the refactor, which should ideally allow for users to compose their own variants based on their needs, without many features depending on being upstreamed to be practical. I'm not against a prop for passing attributes through, as that's more controlled.

A wrapper component can then be used like this(I understand it's not really anymore convenient than passing/extracting props from a dummy component or props object), using omit from lodash:

import Image from "gatsby-image"

export const Img = (props) => {
  const customProps = _.omit(props, Image.propTypes)
  
  return (<Image {...props} passthroughProps={customProps} />)
}
// ... import `Img` and use:
<Img fluid={fluid} data-test="Hello World" />

or

import Image from "gatsby-image"
// Not particularly advised approach, could pass as a prop instead of a child
export const Img = (props) => {
  const attributes = props.children[0].props
  const customProps = _.omit(attributes, Image.propTypes)
  
  return (<Image {...props} passthroughProps={customProps} />)
}
// ... import `Img` and use:
<Img fluid={fluid}>
  <ImgAttrs data-test="Hello World" />
</Img>

Something like that should avoid any issues that @ooloth raises, due to using a higher order component. The next version of gatsby-image might provide some variants and/or upstreamed sub-components that can more flexibly meet the needs of users without expanding the core component to cover a wide range of use cases adding to it's weight and technical debt(this may be minor, but it's the sum total of all the minor features and how that impacts maintenance/contributions going forward).

[wardpeet]

Thank you for opening this issue.

Gatsby-image was created so people can have an easy way to setup images from Gatsby. If we enable custom props, this could lead to bad a11y, weird bugs and more. For these reasons, I'll close this PR. We hope to have a better gatsby-image component in the near future that can be used more granular and fixes your issue.

[polarathene]

For anyone interested in the workaround and caveats, you can find a snippet example here.

You should be able to import that instead and substitute the gatsby-image component with the wrapped version without breakage.

FWIW, I tried to make a good case for it, and how it should be minimal maintenance burden(potential for "weird bugs"), better DX(I assume this was meant over a11y, otherwise I'm confused what was implied as bad a11y) vs unfortunate workaround.

I know that the bundling of attributes into an object prop isn't the best DX, but it does alleviate the maintenance concerns (as someone who has regularly contributed to and reviewed gatsby-image features). Unfortunately, I can only do so much, and in this case my opinions held little weight over core dev decisions.

---

@ooloth is the workaround ok for you? You could assign a fixed className, similar to how gatsby-image-wrapper is handled, then find/replace the codebase to switch out the class being targeted? I'm not sure if any issues beyond that would be run into, beyond what my linked comment mentions.

[ooloth]

@polarathene Thanks!

Passing props/attributes to a wrapper element is a workable solution for now (I didn't realize how complicated the pass-through request would turn out to be).

It would be awkward as a permanent approach, but @wardpeet mentioned there's a broader solution in the works, so I'm looking forward to that.