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

fix: definePageMeta name overriding localized route name #2603

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented Dec 6, 2023

🔗 Linked issue

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

While trying to figure out bundler plugins I figured there was an alternative way to tackle #2581. Maybe this is easier to break by Nuxt updates, but I feel this solution makes more sense than #2599. I still need to test this with disabled routes, and I may be missing other edge cases, please let me know.

What this PR does is transform the generated routes.mjs file, instead letting name defined in definePageMeta override the localized name (see source code), we capture the locale route name suffix and combine it with the name if it exists, this makes named routes work as expected.

So in routes.mjs the generation would be changed like this example

-name: historyeTgp8UZZfFMeta?.name ?? "history___ja",
+name: (historyeTgp8UZZfFMeta?.name ? historyeTgp8UZZfFMeta?.name + "___ja" : undefined) ?? "history___ja",

What do you think @kazupon?

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@BobbieGoede BobbieGoede marked this pull request as ready for review December 6, 2023 21:19
@BobbieGoede BobbieGoede changed the title WIP: fix: definePageMeta name overriding localized route name fix: definePageMeta name overriding localized route name Dec 6, 2023
@BobbieGoede BobbieGoede self-assigned this Dec 6, 2023
@BobbieGoede BobbieGoede requested a review from kazupon December 6, 2023 21:20
@BobbieGoede BobbieGoede force-pushed the fix/define-page-meta-name-localization branch from 6a38c86 to ecfba48 Compare December 7, 2023 00:07
@kazupon
Copy link
Collaborator

kazupon commented Dec 7, 2023

@BobbieGoede Thanks!

What this PR does is transform the generated routes.mjs file, instead letting name defined in definePageMeta override the localized name (see source code), we capture the locale route name suffix and combine it with the name if it exists, this makes named routes work as expected.

So in routes.mjs the generation would be changed like this example

-name: historyeTgp8UZZfFMeta?.name ?? "history___ja",
+name: (historyeTgp8UZZfFMeta?.name ? historyeTgp8UZZfFMeta?.name + "___ja" : undefined) ?? "history___ja",

Your PR looks good!

However, this approach is more of a workaround & hack.
If possible, we hope that work-around this issue if there is a correct way to extend the URL of the named page on the nuxt.

@danielroe
Do you have any solution or idea?

@BobbieGoede
Copy link
Collaborator Author

BobbieGoede commented Dec 7, 2023

@kazupon

However, this approach is more of a workaround & hack.
If possible, we hope that work-around this issue if there is a correct way to extend the URL of the named page on the nuxt.

I agree it does feel hacky, my initial solution was #2599 but the drawback is that users would have to avoid using definePageMeta({ name: '' }) and instead use defineI18nRoute({ name: '' }).

The solution in this PR requires no changes in code or usage for users, but the drawback being that it's more likely to break.

Personally I think the best solution would be to change the generated file from Nuxt's side, the way some data from definePageMeta overrides other values is inconsistent as you can see here. For some reason it's just name and path that overwrite resolved page values, while alias, meta, and redirect overwrite values extracted from definePageMeta.

I think I'll just open a PR to make this more consistent, but I can imagine changing this could be breaking for existing Nuxt projects. 🤔 This change is not as straightforward as I expected, curious to hear Daniel's perspective.

@danielroe
Copy link
Contributor

I think we might consider reworking Nuxt implementation so we actually expose some page metadata at build-time (via pages:extend).... That will solve some edge cases like this (and with typed pages as well, where a custom name matters).

@BobbieGoede
Copy link
Collaborator Author

Closing this as it would break if changes are made upstream, will keep an eye on nuxt/nuxt#24770 and if I have time later (maybe next year 😅) I'll see if I can work on resolving that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants