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

refactor!: replace onlyOnNoPrefix and onlyOnRoot with redirectOn #1210

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

divine
Copy link

@divine divine commented Jun 23, 2021

Trying to make this PR sensible because enabling detectBrowserLanguage.onlyOnRoot here by default creates a lot of failing tests thus making this PR huge.

@rchl
Copy link
Collaborator

rchl commented Jun 24, 2021

Aren't we losing support for a certain behavior now?

Previously there were three options (if we assume alwaysRedirect is false):

  • default behavior - redirect on all URLs
  • onlyOnRoot - redirect only from root
  • onlyOnNoPrefix - redirect only when no prefix

Now the onlyOnRoot options combines the two last cases.

Even if I said to "combine" those two options, I didn't mean to combine the behavior but combine those two separate options into one option that the user can use to selectively enable each behavior. So something like redirectOn: 'root' | 'no-prefix' (not entirely convinced on the naming here but you get the idea).

@divine divine marked this pull request as draft July 17, 2021 17:35
@rchl rchl force-pushed the next branch 2 times, most recently from b23ec96 to e79801c Compare July 17, 2021 21:17
@divine
Copy link
Author

divine commented Jul 18, 2021

So something like redirectOn: 'root' | 'no-prefix'

Hello,

I still think that we should merge both options ( 'root' | 'no-prefix') into one because it's indeed a bug fix or add another lax option which will combine two of them into one as well.

@dword-design you've worked on adding that feature, what do you think?

Thanks!

@divine divine force-pushed the combine-onlyonnoprefix-into branch from 53f0b19 to 117152f Compare July 18, 2021 10:27
@rchl
Copy link
Collaborator

rchl commented Jul 18, 2021

I still think that we should merge both options ( 'root' | 'no-prefix') into one because it's indeed a bug fix or add another lax option which will combine two of them into one as well.

I was under impression that no-prefix (old name onlyOnNoPrefix) is worse for SEO since it might result in crawler being redirected from path like /foo to en/foo while with root (onlyOnRoot) the issue would only exist for the root path.

Here is current description of the options:

  • onlyOnRoot (default: false) - Set to true (recommended for improved SEO) to only attempt to detect the browser locale on the root path (/) of the site. Only effective when using strategy other than 'no_prefix'.
  • onlyOnNoPrefix (default: false) - This is a more permissive variant of onlyOnRoot that will attempt to detect the browser locale on the root path (/) and also on paths that have no locale prefix (like /foo). Only effective when onlyOnRoot is not enabled and using strategy other than 'no_prefix'.

@dword-design
Copy link

dword-design commented Jul 18, 2021

@divine Hey, not sure where I am helpful here, sounds more like an architectural issue 😄. But I will help if needed.

Merging currently does not make sense to me because root means it should only redirect on root and nothing else, while no-prefix redirects on all routes without a prefix (possibly including root). Which is distinctive behavior.

Which of both you think is a bugfix?

@divine divine changed the title refactor!: combine onlyOnNoPrefix into onlyOnRoot refactor!: combine onlyOnNoPrefix and onlyOnRoot Jul 19, 2021
@divine divine force-pushed the combine-onlyonnoprefix-into branch from 117152f to eeb535b Compare July 19, 2021 12:41
@divine divine force-pushed the combine-onlyonnoprefix-into branch from eeb535b to 5191b00 Compare July 19, 2021 12:57
@divine
Copy link
Author

divine commented Jul 19, 2021

Sorry guys, I misunderstood that issue!

@rchl let me know if anything else is needed.

Thanks!

@divine divine marked this pull request as ready for review July 19, 2021 13:11
@divine divine requested a review from rchl July 19, 2021 20:15
docs/content/en/migrating.md Outdated Show resolved Hide resolved
docs/content/en/options-reference.md Outdated Show resolved Hide resolved
src/helpers/constants.js Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
test/module.test.js Outdated Show resolved Hide resolved
src/templates/plugin.main.js Outdated Show resolved Hide resolved
src/templates/plugin.main.js Outdated Show resolved Hide resolved
@rchl
Copy link
Collaborator

rchl commented Jul 19, 2021

There are still 2 references to onlyOnRoot in documentation.

@rchl
Copy link
Collaborator

rchl commented Jul 19, 2021

There are still 2 references to onlyOnRoot in documentation.

Actually that's in the SEO page so should merge next before updating those.

@divine divine force-pushed the combine-onlyonnoprefix-into branch from c198958 to 30f1e62 Compare July 19, 2021 22:17
@rchl
Copy link
Collaborator

rchl commented Jul 19, 2021

There are still 2 references to onlyOnRoot in documentation.

Actually that's in the SEO page so should merge next before updating those.

No, sorry, it's actually the browser-language-detection.md file that still references onlyOnRoot.

@divine divine force-pushed the combine-onlyonnoprefix-into branch from c1676fc to 2cda4be Compare July 19, 2021 22:46
@rchl rchl changed the title refactor!: combine onlyOnNoPrefix and onlyOnRoot refactor!: replace onlyOnNoPrefix and onlyOnRoot with redirectOn Jul 20, 2021
@rchl rchl merged commit ffeaa7e into nuxt-modules:next Jul 20, 2021
@rchl
Copy link
Collaborator

rchl commented Jul 20, 2021

Thanks!

@divine divine deleted the combine-onlyonnoprefix-into branch July 20, 2021 13:40
@rchl rchl mentioned this pull request Jul 29, 2021
10 tasks
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

3 participants