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

Initial support for metadata #44729

Merged
merged 44 commits into from Jan 20, 2023
Merged

Initial support for metadata #44729

merged 44 commits into from Jan 20, 2023

Conversation

shuding
Copy link
Member

@shuding shuding commented Jan 9, 2023

This PR implements page and layout exported metadata field support with limited properties.

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • e2e tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have a helpful link attached, see contributing.md

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Jan 9, 2023
Copy link
Contributor

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

Going to continue to review but left a few comments for now that might be helpful

packages/next/src/lib/metadata/resolve-metadata.tsx Outdated Show resolved Hide resolved
packages/next/src/lib/metadata/resolve-title.ts Outdated Show resolved Hide resolved
packages/next/src/build/webpack/loaders/next-app-loader.ts Outdated Show resolved Hide resolved
packages/next/src/lib/metadata/resolve-metadata.tsx Outdated Show resolved Hide resolved
@shuding shuding marked this pull request as ready for review January 13, 2023 15:29
@shuding shuding changed the title WIP: Support for metadata Support for metadata Jan 13, 2023
Copy link
Contributor

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

We didn't specify the exact merging behavior of metadataBase but I provided a starting point that I think is correct. I'd like you to think about whether this is sensible and propose an alternative if you see problems with the approach

): ResolvedMetadata['openGraph'] {
const url = openGraph
? typeof openGraph.url === 'string'
? new URL(openGraph.url)
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 a case where metadataBase should apply I believe. It is my understanding that urls in og tags need to be fully qualified. The logic maybe should be something like this

// this URL does not have a trailing slash but we should normalize it to assume it always ends in a "directory" not a "file" since we are going to extend it
baseURL = new URL("https://foo.example/base")
// normalize to directory
if (!baseURL.pathName.endsWith('/')) {
  baseURL.pathname = baseURL.pathname + '/';
}

url = new URL(urlStringOrURL, baseURL);

This should map the following

baseURL = new URL("https://foo.example/base")
"bar" => new URL("https://foo.example/base/bar")
"/bar" => new URL("https://foo.example/base/bar")
"./bar" => new URL("https://foo.example/base/bar")
"../bar" => new URL("https://foo.example/bar")
"https://bar.example/bar" => new URL("https://bar.example/bar")
new URL("https://bar.example/bar") => new URL("https://bar.example/bar")

Copy link
Member

Choose a reason for hiding this comment

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

🙏 Will address the metadataBase in the following PRs! thanks for the details 👍

packages/next/src/lib/metadata/resolve-opengraph.ts Outdated Show resolved Hide resolved
packages/next/src/lib/metadata/resolve-opengraph.ts Outdated Show resolved Hide resolved
@huozhi huozhi requested review from gnoff and huozhi January 18, 2023 23:11
Copy link
Contributor

@gnoff gnoff 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 you're basically there, just think you should get rid of the title cast since it is not necessary and may make future changes harder

@gnoff
Copy link
Contributor

gnoff commented Jan 19, 2023

@huozhi @shuding The head.js implementation had some kind of integration into the router cache so that it would be updated as one would expect when transitioning from route to route. Does this need a similar kind of special case in the router?

cc @timneutkens you should probably give this a thumbs up from this perspective at least before we merge

@huozhi huozhi requested a review from gnoff January 19, 2023 00:57
@timneutkens
Copy link
Member

cc @timneutkens you should probably give this a thumbs up from this perspective at least before we merge

Hopped on a call with @huozhi and added support for client-side navigation! All good from my side after Jiachi added the copy of the head.js tests that are passing 💯

@kodiakhq kodiakhq bot merged commit d1f6a42 into canary Jan 20, 2023
@kodiakhq kodiakhq bot deleted the shu/7s22 branch January 20, 2023 08:47
ijjk added a commit that referenced this pull request Jan 20, 2023
ijjk added a commit that referenced this pull request Jan 20, 2023
padmaia added a commit to padmaia/next.js that referenced this pull request Jan 21, 2023
kodiakhq bot pushed a commit that referenced this pull request Jan 21, 2023
…)" (#45130)

This reverts commit 259cbc1.

It does not have necessary changes to make Turbopack work.



## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] [e2e](https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs) tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have a helpful link attached, see [`contributing.md`](https://github.com/vercel/next.js/blob/canary/contributing.md)

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)
ijjk added a commit that referenced this pull request Jan 23, 2023
kodiakhq bot pushed a commit that referenced this pull request Jan 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants