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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 141 additions & 1 deletion integration/meta-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ test.describe("meta", () => {
files: {
"app/root.jsx": js`
import { json } from "@remix-run/node";
import { Meta, Links, Outlet, Scripts } from "@remix-run/react";
import { Links, Meta, Outlet, Scripts } from "@remix-run/react";

export const loader = async () =>
json({
Expand Down Expand Up @@ -119,3 +119,143 @@ test.describe("meta", () => {
).toBeTruthy();
});
});

test.describe("meta array syntax", () => {
let fixture: Fixture;
let appFixture: AppFixture;

// disable JS for all tests in this file
// to only disable them for some, add another test.describe()
// and move the following line there
test.use({ javaScriptEnabled: false });

test.beforeAll(async () => {
fixture = await createFixture({
files: {
"app/root.jsx": js`
import { json } from "@remix-run/node";
import { Links, Meta, Outlet, Scripts } from "@remix-run/react";

export const loader = async () =>
json({
description: "This is a meta page",
title: "Meta Page",
contentType: undefined,
});

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" },
];

export default function Root() {
return (
<html lang="en">
<head>
<Meta />
<Links />
</head>
<body>
<Outlet />
<Scripts />
</body>
</html>
);
}
`,

"app/routes/index.jsx": js`
export const meta = ({ data }) => [
{ title: 'Override Title' },
];

export default function Index() {
return <div>This is the index file</div>;
}
`,
},
});

appFixture = await createAppFixture(fixture);
});

test.afterAll(() => appFixture.close());

test("empty meta does not render a tag", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

await expect(app.getHtml('meta[property="og:type"]')).rejects.toThrowError(
'No element matches selector "meta[property="og:type"]"'
);
});

test("meta { charset } adds a <meta charset='utf-8' />", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(await app.getHtml('meta[charset="utf-8"]')).toBeTruthy();
});

test("meta { title } adds a <title />", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(await app.getHtml("title")).toBeTruthy();
});

test("meta { title } overrides a <title />", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(await app.getHtml("title")).toBe("<title>Override Title</title>");
});

test("meta { 'og:*' } adds a <meta property='og:*' />", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(await app.getHtml('meta[property="og:image"]')).toBeTruthy();
});

test("meta { description } adds a <meta name='description' />", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(await app.getHtml('meta[name="description"]')).toBeTruthy();
});

test("meta { refresh } adds a <meta http-equiv='refresh' content='3;url=https://www.mozilla.org' />", async ({
page,
}) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/");

expect(
await app.getHtml(
'meta[http-equiv="refresh"][content="3;url=https://www.mozilla.org"]'
)
).toBeTruthy();
});

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);
});
});
92 changes: 92 additions & 0 deletions packages/remix-react/__tests__/meta-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import React from "react";
/* eslint-disable @typescript-eslint/no-unused-vars */
import { renderToString } from "react-dom/server";
import "@testing-library/jest-dom/extend-expect";

import { processMeta } from "../components";
import type { HtmlMetaDescriptor, MetaFunction } from "../routeModules";

describe("meta", () => {
it(`renders proper <meta> tags`, () => {
function meta({ data }): HtmlMetaDescriptor {
return {
charset: "utf-8",
description: data.description,
"og:image": "https://picsum.photos/200/200",
"og:type": data.contentType, // undefined
title: data.title,
};
}
function meta2({ data }): HtmlMetaDescriptor[] {
return [
{ name: "description", content: "override 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",
},
{ key: "title", content: "Updated title" },
{ key: "charset", content: "utf-16" },
{ name: "viewport", content: "width=device-width, initial-scale=1" },
];
}

let map = getMeta(
{
title: "test title",
description: "test description",
},
[meta, meta2]
);

// let rendered = renderMeta(map);
// let html = renderToString(rendered);
// console.log(html);

// title should override the title from the first meta function
expect(map.get("title")!.content).toBe("Updated title");
// viewport should be added
expect(map.get("viewport")!.content).toBe(
"width=device-width, initial-scale=1"
);
});
});

type MetaMap = Map<string, HtmlMetaDescriptor>;

function getMeta(data: any, metaFunctions: MetaFunction[]) {
let meta: MetaMap = new Map();
metaFunctions.forEach((metaFunction) => {
let routeMeta = metaFunction({
data,
parentsData: {},
params: {},
// @ts-expect-error
location: null,
});
if (routeMeta) {
processMeta(meta, routeMeta);
}
});

return meta;
}

/* eslint-disable-next-line @typescript-eslint/no-unused-vars */
function renderMeta(meta: MetaMap) {
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} />;
})}
</>
);
}
117 changes: 85 additions & 32 deletions packages/remix-react/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,8 @@ function PrefetchPageLinksImpl({
);
}

type MetaMap = Map<string, HtmlMetaDescriptor>;

/**
* Renders the `<title>` and `<meta>` tags for the current routes.
*
Expand All @@ -697,7 +699,7 @@ export function Meta() {
let { matches, routeData, routeModules } = useRemixEntryContext();
let location = useLocation();

let meta: HtmlMetaDescriptor = {};
let meta: MetaMap = new Map();
let parentsData: { [routeId: string]: AppData } = {};

for (let match of matches) {
Expand All @@ -712,52 +714,103 @@ export function Meta() {
typeof routeModule.meta === "function"
? routeModule.meta({ data, parentsData, params, location })
: routeModule.meta;
Object.assign(meta, routeMeta);
if (routeMeta) {
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} />;
      })}
    </>
  );

<>
{Object.entries(meta).map(([name, value]) => {
if (!value) {
return null;
{Array.from(meta.entries()).map(([key, descriptor]) => {
let { name, property, content } = descriptor;
if (key === "title" && typeof content === "string") {
return <title key={key}>{content}</title>;
}

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

if (name === "title") {
return <title key="title">{String(value)}</title>;
if (property !== undefined) {
return <meta key={key} property={property} content={content} />;
}

// Open Graph tags use the `property` attribute, while other meta tags
// use `name`. See https://ogp.me/
let isOpenGraphTag = name.startsWith("og:");
return [value].flat().map((content) => {
if (isOpenGraphTag) {
return (
<meta
property={name}
content={content as string}
key={name + content}
/>
);
}

if (typeof content === "string") {
return <meta name={name} content={content} key={name + content} />;
}

return <meta key={name + JSON.stringify(content)} {...content} />;
});
if (name !== undefined) {
return <meta key={key} name={name} content={content} />;
}
return <meta key={key} {...descriptor} />;
})}
</>
);
}

/*
* 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.

meta: MetaMap,
routeMeta: HtmlMetaDescriptor | HtmlMetaDescriptor[]
) {
// normalize routeMeta to array format
let items: HtmlMetaDescriptor[] = Array.isArray(routeMeta)
? routeMeta.map((item) =>
item.title
? { key: "title", content: item.title }
: { key: generateKey(item), ...item }
)
: Object.entries(routeMeta)
.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" />

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,
}))
: typeof value === "object"
? {
key: `${key}.${(value as Record<string, string>)?.content}`,
...(value as Record<string, string>),
}
: {
key: propertyName === "name" ? key : `${key}.${value}`,
[propertyName]: key,
content: assertString(value),
};
})
.flat();

// add items to the meta map by key (only add if content is defined)
items.forEach(({ key, ...rest }) => {
if (key && rest && rest.content) {
meta.set(key, rest);
}
});
}

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) {
if (item.key) return item.key;
if (item.title) return "title";
if (item.charset || item.charSet) return "charset";
if (item.name) return item.name;
if (item.property) return `${item.property}.${item.content}`;
return JSON.stringify(item); // generate a reasonable key
}

/**
* Tracks whether Remix has finished hydrating or not, so scripts can be skipped
* during client-side updates.
Expand Down
Loading