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.

Sign up for GitHub

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

Jump to bottom

fix(nuxt): store augmented pages with route path #27823

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix(nuxt): store augmented pages with route path #27823

wants to merge 3 commits into from
6 −5

Conversation

manniL
Copy link
Member

@manniL manniL commented Jun 25, 2024 •
edited by danielroe
Loading

🔗 Linked issue

resolves #27814

📚 Description

Before, when augmenting pages via scanPageMeta, we cached the already-augmented pages based on the file path. When to different routes used the same file, this would lead to not augmenting the second route. This PR caches them by using file path and route path as key

@stackblitz StackBlitz
Copy link

stackblitz bot commented Jun 25, 2024

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

@danielroe
Copy link
Member

danielroe commented Jun 25, 2024 •
edited
Loading

One complex issue here is how we should support users overriding config set in definePageMeta in pages:extends. At the moment this is possible fully. With this change, it no longer becomes possible if you modify the page path at all, meaning it is no longer possible to override the path a page sets in its definePageMeta. This might be an issue with @nuxtjs/i18n for example. (cc: @BobbieGoede)

(Previously it was possible by checking what the scanned page meta was for a page before adding it as a route somewhere else.)

@BobbieGoede
Copy link
Member

With this change, it no longer becomes possible if you modify the page path at all, meaning it is no longer possible to override the path a page sets in its definePageMeta. This might be an issue with @nuxtjs/i18n for example.

Yes, if I understand correctly, this would be a problem 😅 Would have to see if i18n tests fail with this change, I find it a bit hard to mentally step through the order of these hooks vs those in the module rn 🤔

@danielroe danielroe marked this pull request as draft June 28, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BobbieGoede BobbieGoede BobbieGoede left review comments

@danielroe danielroe danielroe approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
3.x bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

layout: false does not remove the layout when multiple routes use the same page component
3 participants
@manniL @danielroe @BobbieGoede