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

docs: document trailingSlash #2546

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BobbieGoede
Copy link
Collaborator

πŸ”— 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

πŸ“ Checklist

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

Copy link

nuxt-studio bot commented Nov 8, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
i18n Edit on Studio β†—οΈŽ View Live Preview 17dc15a

Comment on lines +116 to +121
## `trailingSlash`

- type: `boolean`
- default: `false`

Whether routes have a trailing slash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very sparse. How does that interact with Nuxt's own trailingSlash option? Does it apply to all routes or only those handled by i18n?

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 agree the description is lacking, still need to familiarize myself with the routing logic more so I can't answer those questions yet! I figured this is still better than leaving it undocumented πŸ˜…..

The code it uses is ported from your code in v7 to vue-i18n-routing, unless it has changed a lot, you may be better able to answer those questions than I am.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Version 7 of this module (for Nuxt 2) doesn't use vue-i18n-routing so I'm not familiar with it.

I'm a bit surprised that trailingSlash option was introduced in context of Nuxt module since IMO this should all be controlled through Nuxt's own setting instead of having two, potentially conflicting, options.

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 understand that v7 doesn't use vue-i18n-routing, it's vue-i18n-routing that is based on routing in v7.

As far as I can tell, the reason for configuring it in the module is because Nuxt 3 got rid of the trailingSlash option. Now it's up to users to create a custom NuxtLink component see: https://nuxt.com/docs/api/components/nuxt-link#overwriting-defaults.

Though 3.8 introduced a way to set these NuxtLink defaults in the nuxt config, so we could consider using that config.

Also not sure what happens if users have multiple custom NuxtLink components with different trailingSlash settings, when and why this would be used, and how we would handle such edge cases πŸ€·β€β™‚οΈ.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Then maybe it's just the way to go and I'm making unnecessary noise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you follow the history in blame, you can find this github issue.
nuxt/nuxt#18806

That issue is added by NuxtLink to improve SEO for internal links.

Follow the issue further and you can find this issue.
nuxt/nuxt#15462

That issue is a proposal to support trailing Slash from Nuxt 2.
https://v2.nuxt.com/docs/configuration-glossary/configuration-router#trailingslash

As you can see from reading the issue, there are several ways we plan to improve trailing Slash, and the trailing Slash support added to NuxtLink appears to be one of them.

nuxt/nuxt#15462 (comment)

As far as I can understand from reading the issue, Nuxt's policy is to support trailing slashes only at the minimum, and other cases will be handled in userland.

So I think it makes sense to provide a tranilingSlash option in nuxt i18n.

/cc @manniL @danielroe
if you have anything to additional comment, please comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can understand from reading the issue, Nuxt's policy is to support trailing slashes only at the minimum, and other cases will be handled in userland.

That's correct. We leave that up to the user (either via defineNuxtLink + options, or via setting defaults for the NuxtLink in the nuxt.config.ts) ☺️

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what trailingSlash would necessarily do in the module (compared to setting it via above mentioned options)

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 wonder what trailingSlash would necessarily do in the module (compared to setting it via above mentioned options)

It influences head tags, the generated routes and the way routes are matched and resolved if I understand correctly.

I will be looking into the use of this option, where it is being used and why, and if its usage can be reduced so it won't conflict with the flexible nature of trailingSlash prop of NuxtLink (if it does). We will probably support setting this option in the future via https://github.com/harlan-zw/nuxt-site-config as well (related #2474).

Let's not get distracted, this PR only (barely) documents an old option, motivation behind it is because someone on Discord asked how to enable trailing slashes in this module and enabling this option satisfied their use case.

Just trying to prevent issues as they pour in πŸ˜…..

@MuhammadM1998 MuhammadM1998 mentioned this pull request Jan 11, 2024
4 tasks
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.

None yet

4 participants