feat(gatsby-image): Add customProps as catchall

[RetroCraft]

Description

Allow gatsby-image to accept custom props. This is important for accessibility (i.e. aria-label and role attributes) along with some other use cases mentioned in #24876.

Documentation

I'm not sure where the appropriate documentation changes would need to go. I assume a one-line

Any other props will be passed on to the root tag of the gatsby-image component.

added to wherever is relevant will do.

@gatsbyjs/learning

Related Issues

Fixes #24876.

[polarathene]

Personally not fond of this approach. If props need to be passed on in a vague manner, it should at least be better contained rather than relying on spreading remaining props.

[wardpeet]

I'm going to move this to draft for now as I'm not 100% sure we want to add this to gatsby-image. For discussion I would like us to move to the issue.

[polarathene]

tagProps isn't a good name, as there are multiple instances of Tag and you're only wanting to use it for the wrapping element. Use wrapperAttrs, that more clearly communicates it's for the wrapper only and is for passing element attributes not component props. attrProps may also be valid since the wrapper is top-level.

You also need to add this to Image.propTypes at the bottom of the file.

[polarathene]

This should be the first prop, place it above all others, otherwise it introduces the possibility to override all prior props for this component.

[polarathene]

Same as other comment, order matters, place this above all other props.

[wardpeet]

Hey! Thanks so much for opening a pull request!

We decided to go not forward with the issue and feature request. So, I'll be closing this PR.

We absolutely want to have you as a contributor, so please take a look at our open issues for ideas, and please reach out to us on Twitter at @gatsbyjs with questions.

We offer pair programming sessions if you’d like to work with one of our maintainers to make another contribution.

Thanks again, and we look forward to seeing more PRs from you in the future! 💪💜

[polarathene]

@wardpeet It should have been ok to go ahead with this if the change request from my review was done. It shouldn't cause any problems or maintenance concerns if done that way.

Related issue stated reasoning for closing as:

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.

I'm not sure what weird bugs or other issues will arise? Originally it was an issue because of how props were being provided to gatsby-image(relying on ...rest destructuring syntax), that is bad and would cause such problems. The controlled prop for receiving attributes to spread onto the wrapping div should not be a problem.

It's not possible to assign aria attributes otherwise? What is the suggested workaround? I believe the related issue had some comments for why relying on additional wrapping layer is undersirable.

[wardpeet]

Oh, I think Github bailed on me. It still had the old code and didn't show a refresh button. I still think this is not a great addition and lots of it can be done with working on the wrapper.

I'll re-open as it seems like some people see it as a valuable option. Note that this will be removed in the next major version.

[wardpeet]

Let's be extra explicit here

Suggested change

	tagProps = {},
	imageTagProps = {},

[polarathene]

It's not for the img element though? It's for the div(default tag) wrapper element. My review suggested wrapperAttrs or attrProps which is more clear what the purpose of this props is(attributes for passing to the top-level element). We have lots of "image"/"tag"/"props" keywords in the file already, so I definitely think it's more clearer in all that keyword noise to be more specific it's about attributes.

[wardpeet]

Can you update tests & documentation please.

Thank you for working on this!

[wardpeet]

Seems like I'm having a bad day... Went a bit more thoroughly through the PR. After properly reading it I don't see a use case for this cause this can easily be negated by a wrapper component and using React composition. I don't see value in adding another prop to the gatsby-image.

import * as React from 'react';
import GatsbyImage from 'gatsby-image';

export default function Image({ tagProps, ...props }) {
	return <div {...tagProps}><GatsbyImage {...props} /></div>
}

[polarathene]

Note that this will be removed in the next major version.

@wardpeet can you elaborate on that? I don't know if there's been any internal discussion about v3 of gatsby-image, I was the one proposing to split it out and convert to smaller composable function components along with using hooks. v3 would ideally provide a compatible default, while being easier to compose variations.

This feature should be fine to keep as the attributes are contained in a prop and only intended for the top-level wrapping element since we prevent arbitrary props being passed down anyway.

[polarathene]

I don't see value in adding another prop to the gatsby-image.

@wardpeet That's layering on another div, which has some disadvantages iirc due to CSS. On the related issue another user cited their own use case.

---

EDIT: I just applied your suggestion of a wrapper:

const Test = ({ className, tagProps, ...props }) => (
	<div className={className} {...tagProps}><Img {...props} /></div>
)
// Previously used `Img` instead of `Test`
const GridCell = styled(Test)`

And my images are wrongly sized now. The 2ndry div wrapper needs the inline styles that gatsby-image-wrapper has applied(namely the position: relative; style), and to disable that from the internal gatsby-image wrapper, which being an inline style needs !important or a style override prop.

That could simply be avoided and be seamless for existing projects that want to add the a11y or any other attributes, without all the other hassle.

---

Before:

With div wrapper:

[wardpeet]

I get that it seems like adding 1 extra prop doesn't seem like a big deal. But see it from our angle, another prop means 1 more codepath to support. It's a big project and these things add up. I'm happy to accept PRs that add features that can't be worked around.

You can still fix your grid example by applying different css. a11Y isn't a concern here cause you can just apply these things on the container.

Stay tuned for a gatsby-image to make these things easier and pluck the things you need.

[polarathene]

But see it from our angle

I am, as someone who contributes to gatsby-image and reviews PRs related to it.

another prop means 1 more codepath to support.

Fair, but this isn't doing much with that prop, it spreads them on the wrapper div, and any additional props on that wrapper div will override those attr props. If any issues were caused by it, it'd be pretty obvious and easy to verify due to the single prop to group all attributes, as opposed to the bad practice initially submitted in the PR.

I'm happy to accept PRs that add features that can't be worked around.
You can still fix your grid example by applying different css.

I don't see that as a valid reason to decline the PR. It solves the issue this user and others have run into, and my simple example shows how your workaround isn't particularly pleasant and caused an unexpected breakage. Not only do I need to modify my own CSS(+abstraction through additional wrapper), I have to modify the internal gatsby-image wrapper inline CSS too...

Any project that wants to later add an attribute like a11y, needs to introduce such a workaround, due to gatsby-image rejecting any other props. I don't need this personally, but I can understand why others would want it, and not want to have to maintain/introduce a hacky workaround fix. It's not a notable maintenance burden for this component with the proposed approach(which isn't ideal for the user, but meets them halfway to respect the maintenance needs of the package).

Stay tuned for a gatsby-image to make these things easier and pluck the things you need.

I ask again. Is this being worked on internally somewhere? I was willing to collaborate on the v3 component, but you're giving me the impression this is being done away from public and will just be revealed at some point...?

[polarathene]

Adding tabIndex attribute is a case I just ran into wanting to use, and currently that requires the following:

// Hacky workaround 'gatsby-image' wrapper not supporting attribute passthrough
// 'className' is for styled-components to correctly target due to splitting props
// 'style' alternatively if 'className' isn't needed(regular inline styles) for
// the top-level wrapper.
// 'null' on internal styles will erase the previous definitions via object spread
// May break if internal styles are updated..
// See: https://github.com/gatsbyjs/gatsby/pull/25283#issuecomment-653834820
// 'internalStyle' is the original 'style' prop, for the 'gatsby-image-wrapper' element
// in the event it's internal logic causes problems, which it might with 'fixed' images.

/* eslint-disable react/jsx-props-no-spreading */
const DoubleWrappedImg = ({ className, style, internalStyle, tagProps, ...props }) => (
  <div
    className={className}
    style={{
      position: "relative",
      overflow: "hidden",
      ...style,
    }}
    {...tagProps}
  >
    <Img {...props} style={{ position: null, overflow: null, ...internalStyle }} />
  </div>
)
/* eslint-enable react/jsx-props-no-spreading */

const GridCell = styled(DoubleWrappedImg)

And then adding tagProps={{tabIndex: data.index}} to my GridCell component props. Note the comment about potential breakage if gatsby-image-wrapper styles are updated. Note that there may be potential breakage that's difficult to resolve with this workaround (I haven't tried a scenario that uses these two props to confirm breakage), as it's not particularly feasible to support the logic outside of the component:

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

Lines 483 to 484 in b5926cc

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

Instead of being convenient with a rather simple and light maintenance PR solution, maintenance burden is raised but considered acceptable since it's shifted onto every user that will ever need such functionality, instead of them leveraging the collective(and far lighter) maintenance cost by having this merged in gatsby-image instead.

I don't imagine this type of need is too uncommon compared to some of the specially catered niche props supported by gatsby-image, so this example and it's caveats are probably useful to add into the official docs for better discovery of the workaround. Or in light of all this, perhaps justification for accepting the PR is reconsidered?

[polarathene]

Another caveat is for handling this logic in fixed images:

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

Lines 594 to 596 in b5926cc

	if (style.display === `inherit`) {
	delete divStyle.display
	}

My snippet only accounted for fluid, you'd need to add the default display rule for fixed with the override logic above as well.

fixed images also set fixed width/height based on the active image(if using art direction, this changes, relying on internal logic to know what the active image is, you'd need to duplicate that).

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

Lines 588 to 590 in b5926cc

	display: `inline-block`,
	width: image.width,
	height: image.height,

I haven't tested if that'd cause any issues with the workaround, so passing another prop for the internal wrapper styles may be sufficient should it be an issue. I'll update the workaround snippet.