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

feat(remix-react): Add array-syntax for meta export #2598

Closed
wants to merge 17 commits into from

Conversation

kiliman
Copy link
Collaborator

@kiliman kiliman commented Apr 1, 2022

This PR adds array-syntax to the meta export.

export const meta: MetaFunction = ({data}) => [
  { name: "description", content: data.description },
  { property: "og:image", content: "https://remix.run/logo.png" },
  { property: "og:type", content: "image/png" },
  { key: "http-equiv:refresh", httpEquiv: "refresh", content: "5;url=https://google.com" },
  { title: data.title },
  { name: "viewport", content: "width=device-width, initial-scale=1" },
];

Copy link
Contributor

@donavon donavon left a comment

Choose a reason for hiding this comment

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

What do you think of my suggestions?

@@ -651,7 +651,7 @@ export function Meta() {
let { matches, routeData, routeModules } = useRemixEntryContext();
let location = useLocation();

let meta: HtmlMetaDescriptor = {};
let meta: HtmlMetaDescriptor[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change meta to a Map

type MetaMap = Map<string, HtmlMetaDescriptor>;
let meta: MetaMap = new Map();

* This function processes a meta descriptor and adds it to the meta array.
* This converts the original object syntax to new array syntax.
*/
export function processMeta(
Copy link
Contributor

Choose a reason for hiding this comment

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

change HtmlMetaDescriptor to this:

export type HtmlMetaDescriptor = {
  key?: string;
  name?: string;
  property?: string;
  title?: string;
  content?: string;
} & Record<string, string | string[] | null | undefined>;

and try this for processMeta. it sets key here

export function processMeta(
  meta: MetaMap,
  routeMeta: HtmlMetaDescriptor | HtmlMetaDescriptor[]
) {
  let items: HtmlMetaDescriptor[] = Array.isArray(routeMeta)
    ? routeMeta
    : Object.entries(routeMeta)
        .map(([key, value]) => {
          if (!value) return [];

          let propertyName = key.startsWith("og:") ? "property" : "name";
          return key === "title"
            ? { key: "title", content: assertString(value) }
            : ["charset", "charSet"].includes(key)
            ? { key: "charset", content: assertString(value) }
            : Array.isArray(value)
            ? value.map((content) => ({
                key: `${key}.${content}`,
                [propertyName]: key,
                content,
              }))
            : { key, [propertyName]: key, content: value };
        })
        .flat();

  items.forEach((item) => {
    let [key, value] = computeKey(item);
    // only set if key doesn't exist (i.e. let the furthest route win)
    if (!meta.has(key)) {
      meta.set(key, value);
    }
  });
}

other needed helpers:

function assertString(value: string | string[]): string {
  if (typeof value !== "string") {
    throw new Error("Expected string, got an array of strings");
  }
  return value;
}

function generateKey(item: HtmlMetaDescriptor) {
  console.warn("No key provided to meta", item);
  return JSON.stringify(item); // generate a reasonable key
}

function computeKey(item: HtmlMetaDescriptor): [string, HtmlMetaDescriptor] {
  let {
    name,
    property,
    title,
    key = name ?? property ?? title ?? generateKey(item),
    ...rest
  } = item;

  return [key, { name, property, title, ...rest }];
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made these changes, but a couple of comments:

  1. I was trying not to change HtmlMetaDescriptor since it's a public API. Your version no longer supports the extended object syntax (ie., httpEquiv). My understanding is that's going to be reverted anyway.

  2. The array syntax no longer supports the shorthand for { title: "my title"} instead requiring { key: "title", content"my title" }. Same with charset. Probably need Ryan to weigh-in on this change.

  3. When you're merging meta map, you said only set if key doesn't exist. However, processMap is called from the root to the child route, so without subsequent sets, you can't override. I just removed the check.

With these changes, my local tests work. Still having issues with the integration test.

Object.assign(meta, routeMeta);
if (routeMeta) {
meta = processMeta(meta, routeMeta);
}
}

parentsData[routeId] = data;
}

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

And here's the rendering part. The keys have been pre-computed in processMeta

  return (
    <>
      {[...meta.entries()].map(([key, value]) => {
        if (key === "title" && typeof value.content === "string") {
          return <title key={key}>{value.content}</title>;
        }
        if (key === "charset" && typeof value.content === "string") {
          return <meta key={key} charSet={value.content} />;
        }
        return <meta key={key} {...value} />;
      })}
    </>
  );

@kiliman kiliman marked this pull request as ready for review April 6, 2022 15:35
@machour
Copy link
Collaborator

machour commented Sep 3, 2022

@kiliman Could you fix conflicts here please?

@machour machour added enhancement New feature or request needs-response We need a response from the original author about this issue/PR labels Sep 3, 2022
@felquis
Copy link

felquis commented Sep 22, 2022

Hello team, I want to mention a scenario I found, where open graph og:image accept multiple metatags with the same name, as described at: https://ogp.me/#array

If a tag can have multiple values, just put multiple versions of the same tag on your page. The first tag (from top > to bottom) is given preference during conflicts.

<meta property="og:image" content="https://example.com/rock.jpg" />
<meta property="og:image" content="https://example.com/rock2.jpg" />

Put structured properties after you declare their root tag. Whenever another root element is parsed, that structured property is considered to be done and another one is started.

For example:

<meta property="og:image" content="https://example.com/rock.jpg" />
<meta property="og:image:width" content="300" />
<meta property="og:image:height" content="300" />
<meta property="og:image" content="https://example.com/rock2.jpg" />
<meta property="og:image" content="https://example.com/rock3.jpg" />
<meta property="og:image:height" content="1000" />

means there are 3 images on this page, the first image is 300x300, the middle one has unspecified dimensions, and the last one is 1000px tall.

Just want to mention that this PR might allow this open graph usage. Thank you.

cc #4260

@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2022

⚠️ No Changeset found

Latest commit: 87594af

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kiliman
Copy link
Collaborator Author

kiliman commented Sep 22, 2022

@machour I've rebased onto latest dev

@felquis I added support for duplicate property names. It dedupes by property+content. If you need to override dupes, you can provide a unique key for each.

export const meta = ({ data }) => [
{ key: "charset", content: "utf-8" },
{ name: "description", content: data.description },
{ property: "og:image", content: "https://picsum.photos/300/300" },
{ property: "og:image:width", content: "300" },
{ property: "og:image:height", content: "300" },
{ property: "og:image", content: "https://picsum.photos/200/200" },
{ property: "og:image", content: "https://picsum.photos/500/1000" },
{ property: "og:image:height", content: "1000" },
{ property: "og:type", content: data.contentType }, // undefined
{ key: "httpEquiv:refresh", httpEquiv: "refresh", content: "3;url=https://www.mozilla.org" },
{ title: data.title },
{ name: "viewport", content: "width=device-width, initial-scale=1" },
];

test("meta supports multiple items with same name/property />", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");
expect(await app.getElement('meta[property="og:image"]')).toHaveLength(3);
});

@chaance
Copy link
Collaborator

chaance commented Oct 28, 2022

It dedupes by property+content. If you need to override dupes, you can provide a unique key for each.

@kiliman This seems like it could be error-prone across nested route boundaries. In the vast majority of cases you want your child route's meta to override the parent's. For this to work would users need to provide keys for every tag, or just tags that support duplicates for array values?

I'm starting to wonder if we shouldn't hold off on this until v2. We're already planning some changes that incorporate this change, but if we continue auto-merging meta in nested routes I think array values introduce some potential gnarly edge cases that will be hard to work around.

.map(([key, value]) => {
if (!value) return [];

let propertyName = key.startsWith("og:") ? "property" : "name";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need to be updated. See #4445

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll review this. Basically the key should be either name or property value. The only time you need an explicit key property is if you have duplicates, or there is no name/property like <meta http-equiv="refresh" />

@chaance
Copy link
Collaborator

chaance commented Oct 28, 2022

Just want to mention that this PR might allow this open graph usage. Thank you.

@felquis Just so you know, you can already do this today with the existing API. You just need to pass an array to the tag's content.

export function meta() {
  return {
    "og:image": [
      "https://picsum.photos/300/300",
      "https://picsum.photos/200/200",
    ],
  };
}

It does get a little weird with structured properties though, since we'd need to reorder those according to the protocol. Array values would help there, but it could get weird with nesting.

@kiliman
Copy link
Collaborator Author

kiliman commented Oct 31, 2022

If we're going to support having duplicate items (e.g. multiple og:image entries), it's going to be difficult to deal with child route merges. How do you know if these entries should be added to existing or overwritten? With single keys, it was easy, since it was always replace.

export function meta() {
  return {
    "og:image": [
      "https://picsum.photos/300/300",
      "https://picsum.photos/200/200",
    ],
  };
}

The problem with this is that you can have multiple og:image:height and it refers to the previous og:image. Refer to my example. These sub props can be interleaved and are position dependent, So even allowing arrays for content woudln't support the (very complex and poorly designed) OpenGraph spec.

So I would either recommend adding keys to allow child routes to override, or add an explicit mode: "append" | "override" prop as needed.

@chaance
Copy link
Collaborator

chaance commented Oct 31, 2022

For all interested, we have an RFC that will address a lot of the issues mentioned in this thread. #4462

@ryanflorence ryanflorence mentioned this pull request Nov 1, 2022
2 tasks
@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Nov 3, 2022
@kiliman
Copy link
Collaborator Author

kiliman commented Nov 17, 2022

Closed in favor of #4610

@kiliman kiliman closed this Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants