Shopify / hydrogen

Hydrogen lets you build faster headless storefronts in less time, on Shopify.
https://hydrogen.shop
MIT License
1.24k stars 246 forks source link

Improve type safety for SEO data utilities #757

Closed davidhousedev closed 1 year ago

davidhousedev commented 1 year ago

WHY are these changes introduced?

This PR brings more consistency to the typing of the seo data utilities. This preserves internal consistency within the module and will hopefully help folks in userland who bring the seo data utility module into their own repos.

WHAT is this pull request doing?

Previously, the collection and product seo data utilities in the demo store inferred the types of the data they returned. In contrast, other seo data utililties annotated the return type with the SeoConfig generic.

I realized that the product and collection utilities were using extracted functions for creating the jsonLd values, but those functions returned the incorrect type. I removed the unnecessary array typing and was then able to add the return typing to collection and product.

Checklist

davidhousedev commented 1 year ago

@frehner ah, got it. I took a look at the releases and thought I saw changes to the demo store in there but I must have misunderstood :). I'll remove the changeset.

frehner commented 1 year ago

Ok, actually, turns out we do need a changeset; but you'll only need to add it to the demo-store template and not any of the packages. Sorry about that!

davidhousedev commented 1 year ago

@frehner @juanpprieto I don't see demo-store on the list of available options when adding a changeset, is there something I'm missing here?

image
frehner commented 1 year ago

Hmm, sorry about this @davidhousedev

After talking a bit, it seems like the recommendation is to make a changeset for the cli package. We should probably document that somewhere.

davidhousedev commented 1 year ago

No worries @frehner ! Updated 👍🏻

frehner commented 1 year ago

Wait something seems weird here - are you targeting the correct branch to merge into? Github is saying Shopify:2023-01 but I think it should just be 2023-01, and should also be running some additional checks?

davidhousedev commented 1 year ago

@frehner I'm targeting the Shopify org repo from my fork, that's why Github is adding the Shopify: prefix. Is it possible that your github actions aren't configured to listen for PRs from forks?

I assumed that Github wouldn't let me push a branch or open a PR in the Shopify org since I'm not a contributor to the repo 🤔

benjaminsehl commented 1 year ago

since I'm not a contributor to the repo 🤔

I mean, obviously that was our first mistake 😉

frehner commented 1 year ago

Is it possible that your github actions aren't configured to listen for PRs from forks?

Good callout, I think you're right. One moment while I update our actions.

frehner commented 1 year ago

Ok, sorry again! @davidhousedev can you pull in the latest from shopify/hydrogen? I've updated our action config and they should run once you've pulled in and updated your PR. Thanks

davidhousedev commented 1 year ago

Gonna re-open this with a branch from my fork, so I can keep my forked 2023-01 up-to-date with the base