Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type safety for SEO data utilities #757

Closed
wants to merge 0 commits into from

Conversation

davidhousedev
Copy link
Contributor

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

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

Copy link
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the changes, it seems you're only making updates to the demo-store implementation, is that correct?

If so, then we don't need a changeset.

Unless you meant to update code in the Hydrogen library itself?

@davidhousedev
Copy link
Contributor Author

@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
Copy link
Contributor

frehner commented Apr 4, 2023

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!

Copy link
Contributor

@juanpprieto juanpprieto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @davidhousedev. Looks great. Let's merge after the changeset is back in 🙏🏼

@davidhousedev
Copy link
Contributor Author

@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
Copy link
Contributor

frehner commented Apr 5, 2023

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 davidhousedev force-pushed the 2023-01 branch 3 times, most recently from 44c9937 to 093f9ac Compare April 5, 2023 20:38
@davidhousedev
Copy link
Contributor Author

No worries @frehner ! Updated 👍🏻

@frehner
Copy link
Contributor

frehner commented Apr 5, 2023

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
Copy link
Contributor Author

davidhousedev commented Apr 5, 2023

@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
Copy link
Member

since I'm not a contributor to the repo 🤔

I mean, obviously that was our first mistake 😉

@frehner
Copy link
Contributor

frehner commented Apr 6, 2023

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
Copy link
Contributor

frehner commented Apr 6, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants