Generate SEO tags refactor for better types #616
Conversation
|
We detected some changes in |
| entries.filter((value) => !!value), | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
I think we can add a type check here for never to make sure we've exhausted all options
cartogram
force-pushed
the
seo-type-improvements
branch
from
2eb2e82
to
d8b03e2
Compare
There was a problem hiding this comment.
👋 I've added a few comments below.
Question: would it be interesting to modify the schema to specify string patterns?
handler: Maybe<`@${string}`>;
url: Maybe<`http${string}`>;
// etc
I'm not sure since perhaps most of this data comes from the SFAPI as string, not inlined 🤔
| } | ||
|
|
||
| return 'image/jpeg'; | ||
| return tagResults.flat().sort((a, b) => a.key.localeCompare(b.key)); | ||
| } | ||
|
|
||
| type MetaTagProps = |
There was a problem hiding this comment.
I understand the following is not part of this PR but perhaps from the previous one. In any case, what's the reason of the many generateTag signature overloads? Does it improve the error vs a simplified type using generics?
export function generateTag<T extends TagKey>(
tagName: T,
input: ComponentPropsWithoutRef<T>,
group?: string,
): CustomHeadTagObject {
// ...
}I see the current overloads show the following errors:
If we simplify the overloads, the errors look more like this:
Which I think is more consistent? 🤔
| | 'BlogPosting' | ||
| | 'Thing'; | ||
|
|
||
| function inferSchemaType(url: Maybe<string> | undefined): SchemaType { |
There was a problem hiding this comment.
This function doesn't seem to be used anywhere nor exported.
| function inferMimeType(url: Maybe<string> | undefined) { | ||
| const ext = url && url.split('.').pop(); | ||
|
|
||
| if (ext === 'svg') { | ||
| return 'image/svg+xml'; | ||
| } | ||
|
|
||
| if (ext === 'png') { |
There was a problem hiding this comment.
Nit: I barely use switch statements but this function seems like the perfect fit for it.
cartogram
force-pushed
the
seo-type-improvements
branch
from
d8b03e2
to
bd836d9
Compare
WHY are these changes introduced?
This PR revisits some typescript improvements I have had on the back-burner.
WHAT is this pull request doing?
I've been struggling to achieve the proper type inference when iterating over the object keys in the SEO config. I experimented with Array methods and using the
isoperator, but the code was harder to follow. By moving the iteration outside of the nested array, it both simplifies things and removes a handful of casts.There is still some more work to do here, but this was a first step that we can merge without issue.
HOW to test your changes?
Everything should still work, specifically SEO information still displays when running the demo-store.
All existing tests should still pass.
Post-merge steps
Checklist