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: detectBrowserLanguage overrides non-root paths #603

Closed
wants to merge 2 commits into from
Closed

fix: detectBrowserLanguage overrides non-root paths #603

wants to merge 2 commits into from

Conversation

urbgimtam
Copy link

This commit tries to fix issue #455.

With the fix:

  • if the user visits the root without prefixes AND does not have a cookie already, it honors the detectBrowserLanguage settings, redirecting to the detected language
  • if the user visits a prefixed non-root page, then the url prefix is honored instead of the browsers language or a saved cookie.

Solution provided by @konr4d. I'm just doing the PR to speed up the process :)

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #603 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #603   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         121    121           
  Branches       31     31           
=====================================
  Hits          121    121

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7525cd7...eeba883. Read the comment docs.

@rchl
Copy link
Collaborator

rchl commented Feb 20, 2020

  • We'd need tests for this.
  • I think it breaks no_prefix strategy since browser locale will not be respected on any non-root path (at least when there is no cookie yet).
  • I'm still not quite sure I feel good about the logic it's trying to enforce. Seeing tests would help me reason about it.
  • Also this needs to made clear in documentation somewhere.

@urbgimtam
Copy link
Author

urbgimtam commented Feb 27, 2020

  • We'd need tests for this.

Yes, I'll try to write a few tests.

  • I think it breaks no_prefix strategy since browser locale will not be respected on any non-root path (at least when there is no cookie yet).

I don't think so, as the url triggers the page to be viewed. When using the no_prefix strategy, nuxt-i18n still generates all routes per language correctly. When requesting the url of a certain language, even without the prefix, the current mechanism should work without any extra effort, as it exists and is not touched.

Of course, it will not redirect to the browser language, but I think that's the whole point - browser language is not always the language required by the user. Even when it is, redirecting seems like "too much magic". But maybe that can be a choice of the developer, when setting up the webapp.

Example scenarios where this might be unexpected, or even work against the end user:

  • In a hotel lobby, where guests are sometimes in machines they do not own. Let's assume the desktop browser may be in German. Guest is British, does not speak German. Website is multilingual, supports English and German.

    1. Guests receives a link by email (let's say some login registration, or whatever). Link points to the English version of a specific page, because the user registered as English speaking.
    2. They click the link, and expect to see the page in English, but are redirected to the German version of the page because it detected the browser language to be German.
    3. As he does not understand German, it will take some time to realize what to do. They have to switch manually to English again to complete the task.
  • In larger websites, not all pages are available in all languages at the same time (if ever), specially if they are managed by different teams. It is not uncommon, specially if they have different products available in different regions. Website supports 4 languages: English, German and Portuguese

    1. Website marketing team makes a social media campaign for product 'X'. Product 'X' webpage is available in English and German but not Portuguese, due to time/translation constraints. However, the campaign is time-sensitive, and must start before the Portuguese is finished.
    2. They post an advertisement on Facebook and Instagram in German, with the German link, for the German market. All is fine.
    3. They post a more international advertisement in English, using the English link, for all remaining markets. All is fine for non-portuguese visitors (as it falls back to English), but for Portuguese it crashes the website:
      1. Link is in English.
      2. Browser language detects language is Portuguese. Writes cookie. Redirects by changing the url directly. Problem is the url in Portuguese does not exist.
  • I'm still not quite sure I feel good about the logic it's trying to enforce. Seeing tests would help me reason about it.

IMO, to always redirect to the browser language or the cookie value previously stored only makes sense when users are requesting the the root url. It is generic and they don't yet know what they are searching for.

Any other url should be treated as-is. Its up to the developer to create a language-switching mechanism (menu, button, dropdown, etc).

  • Also this needs to made clear in documentation somewhere.

Of course.

@rchl
Copy link
Collaborator

rchl commented Mar 29, 2020

  • I think it breaks no_prefix strategy since browser locale will not be respected on any non-root path (at least when there is no cookie yet).

I don't think so, as the url triggers the page to be viewed.

Sorry for not getting back to you earlier. There was so much text that I was delaying answering this forever. :)

So you seem to be saying that it will work and then you are also saying later that it won't (Of course, it will not redirect to the browser language). So I'm not sure what you mean.

But to clarify things that I think you might be wrong about (or maybe I'm understanding you wrong).

When using the no_prefix strategy, nuxt-i18n still generates all routes per language correctly.

With no_prefix strategy, there is only a single route generated per-page and all languages use it. The language is determined on loading the page and set accordingly.

When requesting the url of a certain language, even without the prefix, the current mechanism should work without any extra effort, as it exists and is not touched.

With no_prefix there is no URL per language. There is one common URL. For example, there is just /simple instead of /en/simple and /fr/simple.

So this change is changing (you could argue whether breaking) language detection when going to such URL. If one goes to /simple, with this change the language will always be the default language while previously it would be one based on the browser language.

You can test that by:

  1. changing your browser's preferred language to french
  2. adding strategy: 'no_prefix' in /test/fixture/base.config.js
  3. running test fixture with yarn dev:basic
  4. going to localhost:3000/simple.

With the change it will be English while without french.

And as for the rest of your comment...

In a hotel lobby, where guests are sometimes in machines they do not own.

Is it really more relevant example than a user using his own computer? The latter seems a more likely and common scenario. Shouldn't we cater for majority if we can't satisfy both at the same time?

In larger websites, not all pages are available in all languages at the same time

That seems like a valid point. If the user went to the page with specific language prefix and got redirected to a non-existing page then it would be bad.

But what about no-prefix strategy, where URLs are the same for every language? Should we treat those differently then? If the user goes to site.com/product-name then it seems like it would make sense to show him that page in his language of choice. Why should we then treat that URL differently than root URL and default to english?

@stale
Copy link

stale bot commented May 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 29, 2020
@rchl
Copy link
Collaborator

rchl commented May 29, 2020

I like the change, just need to work on tests and changing behavior a bit, according to the comments.

I'll work on that later.

@ems1985
Copy link

ems1985 commented Jun 18, 2020

Check out the discussion in #761 and my proposed solution: bitbuyAT@6228014

I'm not very familiar with the nuxt-i18n code, so my solution may not be ideal or work for every single configuration/strategy, but by adding a detectBrowserLanguage.onlyRedirectFromRoot option my changes should be at least non-breaking. So maybe you could do something similar.

In a later version you could just change the default value for detectBrowserLanguage .onlyRedirectFromRoot or remove it altogether to get the new behaviour (which is how it should be in the first place, from SEO standpoint) as default.

@divine
Copy link

divine commented Jun 20, 2020

@rchl can we have your input on this, please? @ems1985 provided a better solution, can we have a fix finally.

@rchl
Copy link
Collaborator

rchl commented Jun 23, 2020

I'll handle this when I have some time but my earlier comments still apply - that solution wouldn't be right for no_prefix strategy. And redirecting on the root page would still be bad for google crawler, even if it's just that one page.

@divine
Copy link

divine commented Jun 23, 2020

I'll handle this when I have some time but my earlier comments still apply - that solution wouldn't be right for no_prefix strategy. And redirecting on the root page would still be bad for google crawler, even if it's just that one page.

I'm still unsure why you're still talking about no_prefix and trying to take care for it.

It simply doesn't make sense.

no_prefix
With this strategy, your routes won't have a locale prefix added

If there is no locale prefix added - it's not even possible to detect locale from route.

The point here is prefix_and_default where locale can be detected from route by passing browser language auto detection or redirection.

Put it simply, whenever I share http://examples/fr/article language should be set to French even if the person browser language is Arabic.

That's the point. Bots can't change cookies and we should just add option to force changing language based on prefixes.

Bot visits:

Visit Prefix Language Cookie Language Detected Language Action
http://example.com Empty Empty English Redirect to English
http://example.com/en English French Arabic Set language to English
http://example.com/fr French Arabic Arabic Set language to French

Thanks!

@rchl
Copy link
Collaborator

rchl commented Jun 23, 2020

If there is no locale prefix added - it's not even possible to detect locale from route.

This is not about detecting locale from route but from saved cookie or browser-supported-locales (reported through HTTP header or navigator.languages).

[I guess I wasn't clear before on what I mean by "redirecting". I meant it in the sense of changing the language of the page (with or without redirecting to another URL)]

If the user visits the prefixed route then I agree we should not redirect to another language.

But when the user visits a non-prefixed route (index or any route when using no_prefix strategy) then we have to do "something".

The proposal here is to redirect when using strategies with prefixes. That's sort of OK but google will still have issue with that one page redirecting.

When using no_prefix strategy, we're not gonna redirect but the detectBrowserLocale logic will kick in anyway and change the language of the page. And we have to also think about how that should work. I suppose in that case we have to change the language always.

@divine
Copy link

divine commented Jun 23, 2020

If the user visits the prefixed route then I agree we should not redirect to another language.

This is the main problem and still I don't see why we are stuck at no_prefix.

The proposal here is to redirect when using strategies with prefixes. That's sort of OK but google will still have issue with that one page redirecting.

When using no_prefix strategy, we're not gonna redirect but the detectBrowserLocale logic will kick in anyway and change the language of the page. And we have to also think about how that should work. I suppose in that case we have to change the language always.

Can't we just apply a "fix" only when prefix_and_default used with additional configuration without breaking up anything like what other libraries are doing?

Thanks!

@ems1985
Copy link

ems1985 commented Jun 23, 2020

I'm pretty sure the workaround I posted would also work for no_prefix strategy, didn't test it though.

Long to medium term I think there should be some discussion about nuxt-i18n default strategy and behaviour with a focus on SEO (we all try build our websites to be found after all). Because currently (without workarounds) the only way get your non-english pages indexed is by not using redirects at all.

@rchl
Copy link
Collaborator

rchl commented Jun 24, 2020

Your solution would disable locale detection for non-root paths. And is it correct to disable language detection for a route like /about? With no_prefix we don't know the intended locale from the URL so we have to (or we should, rather), detect the locale of the browser.

But it's easy enough to exclude no_prefix strategy from that logic so we don't need to discuss it further.

@ems1985
Copy link

ems1985 commented Jun 24, 2020

Ok, I think I misunderstood how the no_prefix strategy works. But understanding how it works now, from a SEO-standpoint that option is pretty bad, since Google or Bing would never even see localised versions of your pages. It is also impossible to get backlinks to a specific language version of your page. So I don't quite get why anyone would even use it or why this option is there in the first place (except for hiding your content from search engines maybe?). And I also think a warning should be added to the docs on the SEO-related negative effects about using that strategy.

But either way, all my solution does is add an option. All settings unchanged, nuxt-i18n would behave exactly as it does now. Behaviour would only change when enabling the onlyRedirectFromRoot option. And that should work for all strategies, even for no_prefix (although it wouldn't make sense to enable it there)

@divine
Copy link

divine commented Jun 24, 2020

Ok, I think I misunderstood how the no_prefix strategy works. But understanding how it works now, from a SEO-standpoint that option is pretty bad, since Google or Bing would never even see localised versions of your pages. It is also impossible to get backlinks to a specific language version of your page. So I don't quite get why anyone would even use it or why this option is there in the first place (except for hiding your content from search engines maybe?). And I also think a warning should be added to the docs on the SEO-related negative effects about using that strategy.

But either way, all my solution does is add an option. All settings unchanged, nuxt-i18n would behave exactly as it does now. Behaviour would only change when enabling the onlyRedirectFromRoot option. And that should work for all strategies, even for no_prefix (although it wouldn't make sense to enable it there)

no_prefix is not about SEO translated pages but only about the functionality of the site.

It doesn't make sense to use no_prefix for SEO purposes as well as trying to detect language from prefix itself (since there is no language prefix?)

Thanks!

@stale
Copy link

stale bot commented Aug 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 23, 2020
@rchl rchl removed the stale label Aug 23, 2020
@williamweckl
Copy link

Any updates on this? Looking forward for it :)

@rchl
Copy link
Collaborator

rchl commented Sep 9, 2020

A variant of this fix was just merged through #799.
Will be released tomorrow morning.

@rchl rchl closed this Sep 9, 2020
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.

None yet

5 participants