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(nuxt): experimental extraPageMetaExtractionKeys #30015

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

harlan-zw
Copy link
Contributor

@harlan-zw harlan-zw commented Nov 22, 2024

🔗 Linked issue

📚 Description

The definePageMeta() macro is a useful way to collect build-time meta about pages, Nuxt itself provides a set list of supported keys which is used to power some of the internal features such as redirects, page aliases and custom paths.

If a Nuxt module wants to collect bespoke build-time data about pages, then it has to rely on either creating its own build-time macro (see defineI18nRoute()) or use other strategies with worst DX (route rules, module config, etc).

<script lang="ts" setup>
definePageMeta({
  foo: 'bar'
})
</script>
export default defineNuxtConfig({
  hooks: {
    'pages:resolved' (ctx) {
      // ❌ foo is not available
    },
  },
})

This PR introduces the experimental extraPageMetaExtractionKeys config which allows modules to specify additional keys that should be processed and exposed.

Some use cases for this build-time data from my modules:

<script lang="ts" setup>
definePageMeta({
  foo: 'bar'
})
</script>
export default defineNuxtConfig({
  experimental: {
    extraPageMetaExtractionKeys: ['foo'],
  },
  hooks: {
    'pages:resolved' (ctx) {
      // ✅ foo is available
    },
  },
})

It's expected that module authors will augment the NuxtPage schema with their custom keys, technically this could be handled by this config but figuring out the types would be cumbersome for the scope of this PR.

I think the configuration of this isn't great if you have a better idea, I tried a hook first but it wasn't any better.

Summary by CodeRabbit

  • New Features

    • Introduced extraPageMetaExtractionKeys for enhanced metadata extraction from pages.
    • Updated page metadata extraction process to support additional keys.
  • Bug Fixes

    • Added tests to ensure accurate extraction of newly configured metadata keys.
  • Documentation

    • Updated "Experimental Features" guide to include instructions on using extraPageMetaExtractionKeys.

@harlan-zw harlan-zw requested a review from danielroe as a code owner November 22, 2024 12:18
Copy link

stackblitz bot commented Nov 22, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

coderabbitai bot commented Nov 22, 2024

Walkthrough

This pull request introduces modifications to the metadata extraction process within the Nuxt framework. Key functions such as resolvePagesRoutes, augmentPages, and getRouteMeta have been updated to support additional extraction keys, enhancing their flexibility in handling page metadata. A new property, extraPageMetaExtractionKeys, is added to the experimental schema, allowing for configuration of these keys. Additionally, a new test case is introduced to validate the extraction of specified metadata keys, ensuring comprehensive testing of the updated functionality.

Changes

File Path Change Summary
packages/nuxt/src/pages/utils.ts - Updated resolvePagesRoutes to include augmentCtx with extraExtractionKeys.
- Modified augmentPages to accept extraExtractionKeys in the context.
- Changed getRouteMeta to accept extraExtractionKeys for dynamic extraction key handling.
packages/nuxt/test/page-metadata.test.ts - Added a test case to verify extraction of extra metadata keys foo and bar from definePageMeta.
packages/schema/src/config/experimental.ts - Introduced extraPageMetaExtractionKeys property in the experimental schema for metadata configuration.
docs/2.guide/3.going-further/1.experimental-features.md - Added documentation for extraPageMetaExtractionKeys in the Nuxt configuration.

Possibly related PRs

Suggested labels

🔨 p3-minor

Poem

Hop, hop, here we go,
Extra keys in tow,
Metadata's now a breeze,
With foo and bar, if you please!
In the schema, a new delight,
Enhancing pages, oh what a sight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
packages/nuxt/test/page-metadata.test.ts (1)

146-162: LGTM! Consider adding more test cases for edge scenarios.

The test effectively validates the basic functionality of extracting configured extra metadata keys. The implementation is clean and demonstrates the feature working as intended.

Consider adding additional test cases to improve coverage:

it('should handle empty array of extra keys', async () => {
  const meta = await getRouteMeta(`
    <script setup>
    definePageMeta({
      foo: 'bar',
      bar: true,
    })
    </script>
  `, filePath, [])
  expect(meta).toEqual({})
})

it('should handle non-existent extra keys', async () => {
  const meta = await getRouteMeta(`
    <script setup>
    definePageMeta({
      foo: 'bar',
    })
    </script>
  `, filePath, ['nonexistent'])
  expect(meta).toEqual({})
})

it('should extract nested objects in extra keys', async () => {
  const meta = await getRouteMeta(`
    <script setup>
    definePageMeta({
      nested: { foo: 'bar', deep: { value: true } }
    })
    </script>
  `, filePath, ['nested'])
  expect(meta).toEqual({
    nested: { foo: 'bar', deep: { value: true } }
  })
})
packages/schema/src/config/experimental.ts (1)

311-319: LGTM! Consider enhancing the documentation with examples.

The implementation looks good and aligns well with the PR objectives. The type definition and default value are appropriate for the experimental feature.

Consider enhancing the documentation with:

  1. Example usage showing how to specify extraction keys
  2. Example of type augmentation for NuxtPage
  3. Code snippet demonstrating how modules can access the extracted metadata

Example documentation addition:

     * Configure additional keys to extract from the page metadata when using `scanPageMeta`.
     *
     * This allows modules to access additional metadata from the page metadata. It's recommended
     * to augment the NuxtPage types with your keys.
     *
+    * @example
+    * ```ts
+    * // nuxt.config.ts
+    * export default defineNuxtConfig({
+    *   experimental: {
+    *     extraPageMetaExtractionKeys: ['customKey']
+    *   }
+    * })
+    *
+    * // types/nuxt.d.ts
+    * declare module '#app' {
+    *   interface PageMeta {
+    *     customKey?: string
+    *   }
+    * }
+    * ```
     *
     * @type {string[]}
packages/nuxt/src/pages/utils.ts (3)

81-81: Prevent potential property conflicts when merging context objects

When spreading augmentCtx into the context object, there is a slight risk of property conflicts if new properties are added to augmentCtx in the future that might overlap with existing keys like pagesToSkip. Consider explicitly specifying the properties to merge to enhance code clarity and maintainability.

Apply this diff to specify properties explicitly:

- await augmentPages(pages, nuxt.vfs, { pagesToSkip: augmentedPages, ...augmentCtx })
+ await augmentPages(pages, nuxt.vfs, {
+   pagesToSkip: augmentedPages,
+   extraExtractionKeys: augmentCtx.extraExtractionKeys,
+ })

162-162: Add documentation for extraExtractionKeys in AugmentPagesContext

Consider adding a comment or JSDoc to the extraExtractionKeys property in the AugmentPagesContext interface. Documenting this property will improve code readability and help other developers understand its purpose.


241-241: Avoid using any in type assertion for extractionKeys

The type assertion as any as (keyof NuxtPage)[] introduces any, which can undermine type safety. To maintain strong typings, consider refining the type without using any. You can adjust the definition of defaultExtractionKeys and ensure that all elements are of type keyof NuxtPage.

Apply this diff to refine the type assertion:

- const defaultExtractionKeys = ['name', 'path', 'props', 'alias', 'redirect'] as const
+ const defaultExtractionKeys = ['name', 'path', 'props', 'alias', 'redirect'] as (keyof NuxtPage)[]

...

- const extractionKeys = [...new Set<string>([...defaultExtractionKeys, ...(extraExtractionKeys || [])])] as any as (keyof NuxtPage)[]
+ const extractionKeys = [...new Set<keyof NuxtPage>([...defaultExtractionKeys, ...(extraExtractionKeys || [])])]
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f68589c and 28f16cd.

📒 Files selected for processing (3)
  • packages/nuxt/src/pages/utils.ts (4 hunks)
  • packages/nuxt/test/page-metadata.test.ts (1 hunks)
  • packages/schema/src/config/experimental.ts (1 hunks)
🔇 Additional comments (3)
packages/schema/src/config/experimental.ts (1)

311-319: Verify integration with scanPageMeta feature.

Since this feature depends on scanPageMeta, let's verify the integration and ensure it works in both compatibility modes.

✅ Verification successful

Let me gather more information about the scanPageMeta feature and its integration.


Let me check one more thing to understand how the page metadata is extracted.


Based on the gathered information, I can now provide the final response:

Integration with scanPageMeta feature is properly implemented

The extraPageMetaExtractionKeys feature is correctly integrated with scanPageMeta:

  • It's used in the page augmentation process through augmentPages function
  • The keys are passed to getRouteMeta for metadata extraction
  • Both features work together in compatibility modes as scanPageMeta resolves to either true or 'after-resolve' based on compatibility version
  • The feature is well-tested as evidenced by the comprehensive test suite in page-metadata.test.ts
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check integration with scanPageMeta feature

# Check if scanPageMeta is referenced in the codebase when using extraPageMetaExtractionKeys
rg -A 5 "extraPageMetaExtractionKeys" 

# Check if both features are used together in tests
rg -A 5 "scanPageMeta.*extraPageMetaExtractionKeys"

# Look for any potential issues in page metadata extraction logic
ast-grep --pattern 'function $_(page, $_, extraPageMetaExtractionKeys) {
  $$$
}'

Length of output: 1263


Script:

#!/bin/bash
# Search for scanPageMeta implementation and usage
rg -A 10 "scanPageMeta"

# Look for page metadata extraction implementation
ast-grep --pattern 'function augmentPages($$$) {
  $$$
}'

# Check for any tests related to page metadata extraction
rg -l "extraPageMetaExtractionKeys.*test|test.*extraPageMetaExtractionKeys"

Length of output: 9756


Script:

#!/bin/bash
# Look for getRouteMeta implementation which is used in augmentPages
ast-grep --pattern 'function getRouteMeta($$$) {
  $$$
}'

# Check if there are any tests for page metadata extraction
rg -A 5 "getRouteMeta"

Length of output: 5672

packages/nuxt/src/pages/utils.ts (2)

74-81: Properly passing extraExtractionKeys through augmentCtx

The code correctly introduces augmentCtx to pass extraExtractionKeys to the augmentPages function calls in resolvePagesRoutes(). This ensures that additional metadata keys are considered during page augmentation.


170-170: Passing extraExtractionKeys to getRouteMeta

The function augmentPages now correctly passes ctx.extraExtractionKeys to getRouteMeta, enabling the extraction of additional page metadata as specified.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
docs/2.guide/3.going-further/1.experimental-features.md (4)

398-402: Consider emphasizing the target audience in the introduction

While the introduction effectively explains the feature's purpose, it would be helpful to explicitly mention that this feature is primarily intended for module authors early in the description.

 ## extraPageMetaExtractionKeys
 
-The `definePageMeta()` macro is a useful way to collect build-time meta about pages. Nuxt itself provides a set list of supported keys which is used to power some of the internal features such as redirects, page aliases and custom paths.
+The `definePageMetaExtractionKeys` feature enables module authors to extend Nuxt's build-time page metadata extraction capabilities. While Nuxt itself provides a set list of supported keys for the `definePageMeta()` macro (used for features like redirects and page aliases), module authors often need to extract additional custom metadata.
 
 This option allows passing additional keys to extract from the page metadata when using `scanPageMeta`.

404-410: Enhance the example with a practical use case and type information

The current example uses a generic foo: 'bar' which doesn't illustrate a real-world scenario. Consider using a more practical example that demonstrates how a module might use this feature.

 <script lang="ts" setup>
+// Example: Custom metadata for Nuxt Sitemap module
+interface PageSitemapMeta {
+  changefreq?: 'always' | 'hourly' | 'daily' | 'weekly' | 'monthly' | 'yearly' | 'never'
+  priority?: number
+  lastmod?: string
+}
+
 definePageMeta({
-  foo: 'bar'
+  sitemap: {
+    changefreq: 'weekly',
+    priority: 0.8,
+    lastmod: new Date().toISOString()
+  } satisfies PageSitemapMeta
 })
 </script>

412-423: Expand the configuration example with practical module usage

The example could better demonstrate how module authors might utilize the extracted metadata in a real-world scenario.

 export default defineNuxtConfig({
   experimental: {
-    extraPageMetaExtractionKeys: ['foo'],
+    extraPageMetaExtractionKeys: ['sitemap'],
   },
   hooks: {
     'pages:resolved' (ctx) {
-      // ✅ foo is available
+      // Example: Generate sitemap.xml during build
+      const sitemapEntries = ctx.pages
+        .filter(page => page.meta.sitemap)
+        .map(page => ({
+          url: page.path,
+          changefreq: page.meta.sitemap.changefreq,
+          priority: page.meta.sitemap.priority,
+          lastmod: page.meta.sitemap.lastmod
+        }))
+      
+      // Module authors can now use this data to generate sitemap.xml
     },
   },
 })

425-426: Expand the type augmentation guidance

The current guidance could better explain the importance of type augmentation and its benefits.

-This allows modules to access additional metadata from the page metadata in the build context. If you are using this within a module, it's recommended also to [augment the `NuxtPage` types with your keys](/docs/guide/directory-structure/pages#typing-custom-metadata).
+This allows modules to access additional metadata from the page metadata in the build context. For module authors, it's strongly recommended to [augment the `NuxtPage` types with your custom keys](/docs/guide/directory-structure/pages#typing-custom-metadata). Type augmentation ensures:
+
+- Better IDE support with autocompletion for your custom metadata keys
+- Type safety when accessing metadata in your module code
+- Improved developer experience for users of your module
packages/nuxt/src/pages/utils.ts (3)

170-170: Consider adding null coalescing for extraExtractionKeys

While the implementation works, it could be more robust by ensuring extraExtractionKeys is always defined when passed to getRouteMeta.

-const routeMeta = await getRouteMeta(fileContent, route.file, ctx.extraExtractionKeys)
+const routeMeta = await getRouteMeta(fileContent, route.file, ctx?.extraExtractionKeys ?? [])

202-207: Consider memoizing the defaultExtractionKeys Set

For better performance, consider creating a Set from defaultExtractionKeys once, rather than on every function call.

-const defaultExtractionKeys = ['name', 'path', 'props', 'alias', 'redirect'] as const
+const defaultExtractionKeys = new Set(['name', 'path', 'props', 'alias', 'redirect'] as const)

303-303: Consider caching the key validation result

The extractionKeys.has() check is performed in a loop. For better performance, consider caching the validation result.

+const validKeys = new Set(extraExtractionKeys.filter(key => extractionKeys.has(key as keyof NuxtPage)))
-if (!extractionKeys.has(name as keyof NuxtPage)) {
+if (!validKeys.has(name)) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 28f16cd and ae13a98.

📒 Files selected for processing (2)
  • docs/2.guide/3.going-further/1.experimental-features.md (1 hunks)
  • packages/nuxt/src/pages/utils.ts (5 hunks)
🔇 Additional comments (3)
packages/nuxt/src/pages/utils.ts (3)

74-81: LGTM: Consistent propagation of extraction keys

The changes correctly propagate the extra extraction keys through different code paths while maintaining the existing functionality.


162-164: LGTM: Clean interface extension

The interface extension is well-typed and maintains backward compatibility by making the new field optional.


227-228: 🛠️ Refactor suggestion

Improve type safety of extraExtractionKeys

The current type assertion as Array<keyof NuxtPage> assumes all extra keys are valid NuxtPage properties. Consider adding runtime validation or using a more specific type.

-const extractionKeys = new Set<keyof NuxtPage>([...defaultExtractionKeys, ...extraExtractionKeys as Array<keyof NuxtPage>])
+type ValidPageKey = keyof NuxtPage
+const extractionKeys = new Set<ValidPageKey>([
+  ...defaultExtractionKeys,
+  ...extraExtractionKeys.filter((key): key is ValidPageKey => {
+    const isValid = typeof key === 'string' && key in ({} as NuxtPage)
+    if (!isValid) {
+      console.warn(`[nuxt] Invalid page meta key: ${key}`)
+    }
+    return isValid
+  })
+])

@danielroe danielroe merged commit 16ef9be into main Nov 27, 2024
45 of 46 checks passed
@danielroe danielroe deleted the feat/experimental-custom-page-meta-key-extraction branch November 27, 2024 16:57
@github-actions github-actions bot mentioned this pull request Nov 27, 2024
@github-actions github-actions bot mentioned this pull request Nov 27, 2024
Kamsou pushed a commit to Kamsou/nuxt that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants