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

Deprecate the Seo component and add getSeoMeta for migration #1875

Merged
merged 9 commits into from
Apr 9, 2024

Conversation

blittle
Copy link
Contributor

@blittle blittle commented Mar 19, 2024

WHY are these changes introduced?

We introduced the <Seo /> component because Remix did not support generating json/ld meta data from route meta exports. This was fixed last year. Additionally, the Seo component is difficult to use with Remix's existing meta. It's difficult to pick and choose and override what you want from it. We think it is simpler to just use Remix meta exports directly.

WHAT is this pull request doing?

This PR deprecates the Seo component and introduces a new helper function to make migration easy.

See the changeset for a complete explanation and examples. Also updated docs are available at: https://shopify-dev.shopify-dev-www3.bret-little.us.spin.dev/docs/api/hydrogen/2024-01/utilities/getseometa

HOW to test your changes?

Add a meta export to routes within the skeleton template. Call and return getSeoMeta with SEO properties. Make sure the proper meta data is generated on the page.

Post-merge steps

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

This comment has been minimized.

Copy link
Contributor

shopify bot commented Mar 19, 2024

Oxygen deployed a preview of your bl-seo-meta branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM
subscriptions ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM
skeleton ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM
vite ✅ Successful (Logs) Preview deployment Inspect deployment April 9, 2024 3:52 PM

Learn more about Hydrogen's GitHub integration.

@blittle blittle force-pushed the bl-seo-meta branch 2 times, most recently from 64cf67d to f43e3b4 Compare March 19, 2024 18:27
/**
* Generate a Remix meta array based on the seo property used by the `Seo` component.
*/
export function getSeoMeta<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this is copied from generate-seo-tags.ts, but slightly modified to return Remix supported objects.

import {getSeoMeta} from './getSeoMeta';
import {SeoConfig} from './generate-seo-tags';

describe('getSeoMeta', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also copied from the existing seo.test.ts and slightly modified.

Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

I think this change makes sense. How are we going to support composing meta from different route segments? Remix right now asks you to do it manually. I guess we should do the same?

packages/hydrogen/src/index.ts Show resolved Hide resolved
@blittle
Copy link
Contributor Author

blittle commented Mar 22, 2024

I think this change makes sense. How are we going to support composing meta from different route segments? Remix right now asks you to do it manually. I guess we should do the same?

Good call. I'll think a bit more about this, and see if there is a simple way we can deal with merging (beyond just copying their utility).

@blittle blittle force-pushed the bl-seo-meta branch 4 times, most recently from 3ed7aa1 to b216437 Compare April 3, 2024 21:51
```ts
export const meta = ({data, matches}) => {
return getSeoMeta(
args.matches[0].data.seo,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is suppose to be just matches[0].data.seo?

Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

I had already approved it but I've now checked the merging streategy. Looks good 👍

packages/hydrogen/src/seo/getSeoMeta.ts Outdated Show resolved Hide resolved
packages/hydrogen/src/seo/getSeoMeta.ts Outdated Show resolved Hide resolved
* Generate a Remix meta array from one or more SEO configuration objects. This is useful to pass SEO configuration for the parent route(s) and the current route. Similar to `Object.assign()`, each property is overwritten based on the object order. The exception is `jsonLd`, which is preserved so that each route has it's own independent jsonLd meta data.
*/
export function getSeoMeta(
...seoInputs: Optional<SeoConfig>[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I meant with the optional callback/transformer. How do you tell typescript "these are all SeoConfigs except the last one"? In the first argument it's easy but the last? 🤔

@blittle blittle merged commit 4afedb4 into main Apr 9, 2024
13 checks passed
@blittle blittle deleted the bl-seo-meta branch April 9, 2024 16:41
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.

None yet

4 participants