solidjs / solid-meta

Write meta tags to the document head
127 stars 16 forks source link

Meta tags with same `name` attribute but different other attributes will be overwritten #34

Closed GoodbyeNJN closed 10 months ago

GoodbyeNJN commented 11 months ago

For example, use the following two tags:

<Meta name="theme-color" media="(prefers-color-scheme: light)" content="#fff" />
<Meta name="theme-color" media="(prefers-color-scheme: dark)" content="#000" />

Only the latter will actually be rendered.

The problem lies here:

https://github.com/solidjs/solid-meta/blob/c9d21efc62fcb4df1205d849b0a3abfa3623fcf8/src/index.tsx#L156-L170

This function seems to remove duplicate tags, but only name or property attributes are compared when checking whether they are duplicates.

I suppose to sort props by key and then serialize it as the unique key of a tag for comparison.

DaniGuardiola commented 9 months ago

This fix broke my website's open graph meta tags.

I had a top level set of meta tags with default, e.g.

<Meta name="og:image" property="og:image" content="default.png" />

And I used child meta tags to override it, e.g.

<Meta name="og:image" property="og:image" content="actual-image.png" />

It worked fine. However, due to this change, it now doesn't override the tag, and both are shown at the same time, causing tools reading the open graph tags to use the first one and not the correct one.

This is because the key now is different for both:

// first key
'meta{"content":"default.png","name":"og:image"}'

// second key
'meta{"content":"actual-image.png","name":"og:image"}'
GoodbyeNJN commented 9 months ago

@DaniGuardiola Sorry for breaking existing features while fixing this issue. I'm reorganizing the attributes of meta element and plan to list out a separate test case for meta element first. Once a separate test case for meta element is approved by the maintainer, I would re-fix this issue.

DaniGuardiola commented 9 months ago

@GoodbyeNJN it's okay, shit happens :)

See https://github.com/solidjs/solid-meta/pull/39 - I think the cascading/overriding effect should just be completely explicit and opt-in because the heuristics get too complicated otherwise.