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

feature: prevent path generation for default language when rewriteDefaultLanguage is false #25

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

Conversation

nuria-fl
Copy link

For a project I'm working on I needed the default language to not have the /en/ path prefix. I checked how this is handled in Nuxt and there are these possible strategies:

- 'no_prefix': routes won't be prefixed
- 'prefix_except_default': add locale prefix for every locale except default
- 'prefix': add locale prefix for every locale
- 'prefix_and_default': add locale prefix for every locale and default

https://nuxt-community.github.io/nuxt-i18n/routing.html#strategy

This PR implements the prefix_and_default as the default, and also adds the option for prefix_except_default.

Let me know how you feel about this implementation, I'm open to any change :) I was unsure if it should throw an error if the strategy name is invalid, or if it should just fallback to the default option (that's what I did).

@nuria-fl
Copy link
Author

nuria-fl commented Jul 21, 2020

I just noticed that, while the en folder is no longer being redirected, the app still redirects to /en/. Setting enablePathRewrite to false seems to solve the issue, how would you handle this? Should it be set to false automatically if the user sets the prefix_except_default strategy?
Or maybe we could ditch the strategy approach and just prevent generating the default language folder if both enablePathRewrite and rewriteDefaultLanguage are set to false (I'm a bit confused about what exactly each option does)

@daaru00
Copy link
Owner

daaru00 commented Jul 24, 2020

Hi @nuria-fl,

not sure to fully understand. Why isn't enough the already provided options enablePathRewrite: false? It will leave the client on default path /, /my-page without redirect to default language like /en/, /en/my-page.

I think something can go wrong here

      const pathSegment = this.setLocalePath(locale)
      this.pagesToGenerate.push({ 
        path: this.mergePathParts(pathSegment, options.path),
        component: route.component,

is path segment has no changes (for example with prefix_except_default) it will generate a path with an already exist page (like /my-page), this should raise a warning and it doesn't seem correct for me.

@nuria-fl
Copy link
Author

not sure to fully understand. Why isn't enough the already provided options enablePathRewrite: false? It will leave the client on default path /, /my-page without redirect to default language like /en/, /en/my-page.

Even when disabling the path rewrite, the /en/ directory gets generated, which I think is not ideal (increases build time, potential problems with duplicated content if you don't specify the canonical url).

is path segment has no changes (for example with prefix_except_default) it will generate a path with an already exist page (like /my-page), this should raise a warning and it doesn't seem correct for me.

I'll take a look at the implementation, I didn't see any warnings but I'll double check!

@daaru00
Copy link
Owner

daaru00 commented Jul 24, 2020

Even when disabling the path rewrite, the /en/ directory gets generated, which I think is not ideal (increases build time, potential problems with duplicated content if you don't specify the canonical url).

totally agree! Perhaps a fix is more appropriate in this case directly fixing this behaviour (like an early return before pushing page into pagesToGenerate array). I'd rather not have to add another option for this purpose. It would be a problem for you?

@nuria-fl
Copy link
Author

Even when disabling the path rewrite, the /en/ directory gets generated, which I think is not ideal (increases build time, potential problems with duplicated content if you don't specify the canonical url).

totally agree! Perhaps a fix is more appropriate in this case directly fixing this behaviour (like an early return before pushing page into pagesToGenerate array). I'd rather not have to add another option for this purpose. It would be a problem for you?

That makes sense! Honestly I started looking into the strategy solution before realising what enablePathRewrite and rewriteDefaultLanguage do. What I'm not sure is, should we prevent the generation of the default language folder if rewriteDefaultLanguage is set to false, or does enablePathRewrite need to be set to false as well? I'm a bit confused about what each one does exactly.

@daaru00
Copy link
Owner

daaru00 commented Jul 28, 2020

What I'm not sure is, should we prevent the generation of the default language folder if rewriteDefaultLanguage is set to false, or does enablePathRewrite need to be set to false as well?

For your use case, you should just set rewriteDefaultLanguage to false and leave enablePathRewrite to the default value (true). This will activate the Vue Router middleware that keep the language during navigation: you can use /my-page or /my-other-page in your links and when navigate it use the default language (en in your case). When you change locale, for example with a select/switch all link now will be redirect (on client side) to /it/my-page or /fr/my-page.

Disabling enablePathRewrite no redirect are performed, even for /it/ and /fr/ and this force you to directly translate the link inside page, using $tp('/my-page/') helper. This give the choice to the developer whether to use a rewrite of the paths or translate the links directly, perhaps for SEO it is better this solution since the correct links are already on the page.

I agree with you that when rewriteDefaultLanguage is disable also page generation should be disabled for default language. The value of enablePathRewrite shouldn't change the page generation behaviour, it just operate at client side rewrite but translated page should be accessible.

@nuria-fl
Copy link
Author

@daaru00 thanks for the explanation, much clearer now :) I'm a bit busy with other projects but I'll try to adjust this in the next couple of weeks!

@nuria-fl nuria-fl force-pushed the feat/skip-path-for-default-locale branch from d5e9fa9 to c088709 Compare August 18, 2020 07:25
@nuria-fl nuria-fl changed the title feature: add strategy option to skip path prefix for default locale feature: prevent path generation for default language when rewriteDefaultLanguage is false Aug 18, 2020
@nuria-fl
Copy link
Author

hi @daaru00, I adjusted the behavior to what we discussed, let me know what you think :)

By the way, I noticed that the readme says that the default value for rewriteDefaultLanguage is true but it doesn't seem to be the case looking at the code now, is that right?

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.

2 participants