Think through data prop feedback

[frehner]

Gather feedback on whether devs think that using a singular data prop is better or worse than individual props with primitives when using Hydrogen-UI/storefront-kit and the Storefront API.

Context:

• Hydrogen-UI (soon to be renamed to storefront-kit) is a collection of React components that are built specifically for the Storefront API and its objects.
• It is not a goal of Hydrogen-UI to be a general-purpose component library; in other words, an <Image/> component is intended to only work with Storefront API Image, a <ProductProvider/> component only works with a Storefront API Product, etc.
• The majority of these components are "leaf nodes"; in other words, they are often the last rendered component in a React tree and do not have children.

Considerations:

• Which is the best DX?
• Are there performance implications by choosing one over the other?
• Do either create patterns in developer's code that we want to encourage or discourage?
• Could we enable both patterns, and if so, are there downsides?

Examples

data prop:

const product = getProductFromStorefrontApi();

return (
  <Image data={product.image}/>
)

Individual props:

const product = getProductFromStorefrontApi();
const {src, width, height, altText} = product.image;

return (
  <Image src={src} width={width} height={height} alt={altText}/>
)

---

(Feel free to add your thoughts to this issue)

[davidhousedev]

+1 for data prop.

I like that it separates the concerns of data derived from the Storefront API vs props that we might supply to customize those components in our own app.

[beppek]

If you add the data prop, is it possible to override certain values?

Say we use imgix to get the images because of its performance improvements, does that mean we shouldn't use the <Image> component?

Does the <Image> component generate a srcset for images?

[beppek]

Also, how does getting product data from sources like Algolia affect this? The data structure could be different. I fear that the data prop assumes that all data always and only comes from Storefront API.

Headless sites will often have a mix of data sources.

[frehner]

@beppek Correct, the <Image/> component is intended to only work with Storefront API Images, and is not meant to be a general purpose component. In the first post, please note these bullet points in the Context section:

Hydrogen-UI (soon to be renamed to storefront-kit) is a collection of React components that are built specifically for the Storefront API and its objects.

It is not a goal of Hydrogen-UI to be a general-purpose component library; in other words, an <Image/> component is intended to only work with Storefront API Image, a <ProductProvider/> component only works with a Storefront API Product, etc.

[frehner]

I created an internal poll, and the overwhelming majority of devs there preferred individual props. As of time of writing:

• 59 votes for individual props
• 4 votes for a data prop
• 4 votes for "both!"

Based on the feedback received, it seems that we should move forward with individual props. It's something that I would like to have done for the 2023-01 release.

[frehner]

Shopify/hydrogen-react#116 is a first pass at this.

But also, the data prop is making a late comeback after some discussion. 🤔

[beppek]

I was going to say that my knee-jerk reaction is to prefer individual props without really thinking about why. I understand the reasoning with the Image component and that the ProductProvider needs the data to match what SF API expects.

I think it's easier for devs to reshape the product data (if it comes from another source) with individual props rather than a single data prop. I also wonder if the single data prop would encourage a dump pattern where devs overfetch data and just dump it all in the data prop, including excessive data that's not used but now passed along to every page where the component is used, causing a double bottleneck of first slowing down the initial server fetch and second slowing down the response to the client.

Curious to hear if there were any arguments for or against data props in your internal poll, or if it was just a simple cast your ballot without any debate?

[frehner]

I think it's easier for devs to reshape the product data (if it comes from another source) with individual props rather than a single data prop.

That's a good point.

And I think the flip-side is also true - it's easier for a developer to get an object from the Storefront API and pass it as a single data prop then it is to have to spread them out.

So I guess it comes down to which experience we're optimizing for.

I also wonder if the single data prop would encourage a dump pattern where devs overfetch data and just dump it all in the data prop, including excessive data that's not used but now passed along to every page where the component is used, causing a double bottleneck of first slowing down the initial server fetch and second slowing down the response to the client.

Yeah, that's a good concern to have. I'm not entirely sure that the component itself is going to help dictate how efficient the query is, though - it seems like the component may be the wrong layer for that (e.g. someone can still just pull out an Image's src prop and pass that, while still querying for all the other fields). I would love for the Storefront API to get some query cost fields added to responses like the Admin API has, and then help optimize at that layer instead. One day, maybe.

Curious to hear if there were any arguments for or against data props in your internal poll, or if it was just a simple cast your ballot without any debate?

Some highlights:

• One good argument for individual props was that the TypeScript DX is better for that, compared to the current situation where we use PartialDeep<Image> for data.
• Individual props could lead to devs using the spread pattern <Image {...product.image}/> which could both be good and bad; good in the sense that for a simple thing, like Image, that works fine, but for a Product, we probably don't want to encourage spreading all those potential props to the component when we probably only need a few props. (Could lead to prop collision - or props eventually getting rendered to the DOM as HTML attributes, as many of these components do pass-through props so you can control the native HTML element as you desire)
• The data prop can be good if the explicit goal of Hydrogen-UI (storefront-kit) is to be a set of components purpose-built for the Storefront API and not for any other APIs or data. It provides for a simpler DX than having to extract properties from those objects just to get the components to work.

[ryanleichty]

My gut reaction is to prefer individual props as it’s more explicit about what you’re passing (less magic ✨). But, to your last point, I see the value of a singular data prop given that the components are purposefully built for the Storefront API.

[benjaminsehl]

Discussed this with @elisenyang — we'd like to proceed with the plan to support both. We will keep the simple mental model of CSS classes vs. inline styles … 

In other words,data is the only prop you need. Sensible defaults are set where data lacks appropriate information. Individual props override data when set, and where appropriate.

This is a better solution than the spread pattern, as certain components may not want to have the value from data provided as the actual prop value … for example, product.featuredImage.width would be the original width … e.g. 4000px … that would be an inappropriate value for the width prop in virtually every use case.

As an example of how the above proposal would play out, I've updated the Image component PR to follow this behaviour. Shopify/hydrogen-react#114

[DavidWittness]

Looks great Ben! I like this approach and I think it'll give developers maximum flexibility.