-
-
Notifications
You must be signed in to change notification settings - Fork 483
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
feat: "onlyOnNoPrefix" - detect browser locale when no prefix #896
Conversation
What's the reason you have removed This shouldn't be working within that strategy at all. |
I thought it‘s not needed anymore because the check is already at the top. But maybe it‘s not redundant.🤔 |
Many tests are failing and also this change doesn't quite make sense to me since if you will prioritize route's locale instead of detecting the browser locale then that will make browser locale detection pretty useless (it will probably only work for non-existant routes). Anyway, are you aware of detectBrowserLanguage.onlyOnRoot option? I think this is what you might want to look into. |
The browser detection will trigger if the route does not include a locale. So /foo and / still use the browser detection. That‘s why there is the null check. Could be that the tests work again when the prefix check is back in. I could check that later. |
I think you are misunderstanding how |
@rchl That's partially right. It indeed checks the locale route via a regex, but also checks the name of the route and this leads to problems in my solution if the strategy is not prefix. I adjusted the implementation to use the regex, which fixes the tests, and I added two tests for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a breaking change and needs to be guarded by another option. Otherwise, it will for example affect
alwaysRedirect
behavior in an unexpected way (I think we don't have tests that cover that case). - Related to above, we need tests for navigating with
i18n_redirected
cookie set to verify that this behavior doesn't trigger withalwaysRedirect
set. And that it does trigger with yet-to-be-added option added. (You can look atfallbacks to default locale with invalid locale cookie
test for inspiration). - The
onlyOnRoot
should probably "win" over this behavior when set, which it is not doing right now (that also pertains to missing option for this behavior).
And a general question: I'd like to know why you think this change makes sense. |
Co-authored-by: Rafał Chłodnicki <[email protected]>
Alright let's have a look. Firstly, I added your suggested code changes. Breaking change: alwaysRedirect: onlyOnRoot: Motivation: Also, there are many websites that as far as I see it go with my approach. For example (without cookies): There are more, but maybe that's fine for the start. I'm actually wondering why it's not the default behavior to be able to navigate between languages without cookies but still using browser detection when not providing a language code. It's probably the use case for all my websites. But requirements are different I guess 😄. |
Given the number of people using this module (or number of people using sites using this module), it's bound to be a breaking change for some. Better play it safe as we did with That said, the plan is to make the recently introduced
Well, first we have to decide what would the option for this behavior look like. Adding another option would likely be confusing as then it's not clear how they interact with each other and make it harder to decide on the correct options to go with. The ideal would probably be to have
Evernote one doesn't seem to redirect at all. Visiting root page (or one of the sub-pages) doesn't redirect me to the Polish version even though I've set Polish as my preferred language.
This one seems to redirect me to the English version, even when having German as my preferred language...
Why not default? Probably some bad decision when creating this module (which was before I've taken over). But as I've said, the plan is to enable Now here is something that I'm concerned about this change... The With So I'm not sure if I really want this change in and also I wonder why |
@rchl Thanks for your elaboration! I'd say we should jump to requirements before we (or I) continue developing. So, here is the set of requirements that I have in mind. If there is a crack in my thoughts, go ahead. The browser locale is
What I actually do not like so much is that root is always redirected because in some cases there are problems when pasting the url into a form and the form validates the existence of the website. In these cases the redirect is considered a problem (but this would also a problem with I had some thoughts of not using a prefix and still showing the website in the browser's language, but this should always break crawling, or am I wrong? Maybe you can comment on the requirements and maybe we could conclude some "cook book" like doc from it, which would make selecting the right config easier 😃. P.S.: It's interesting that the redirect does not work for the two examples. It could be that the language detection works differently. In my case it both redirects to |
This wouldn't happen because
So maybe consider using the cookie to help in those real-user cases. It shouldn't affect the crawlers since crawlers ignore cookies.
Yes, that wouldn't be crawler-friendly. There is a
Which examples? Maybe you should create a repo that reproduces your situation so that we are on the same page? |
Hey @rchl, the behaviour @dword-design is trying to implement is a function I need as well. My website uses multiple languages and my idea is to send users links to pages without a locale (eg. /privacy). The behaviour I'm expecting is that the user opens the link and then is redirected to his/hers browser-language. At the moment this function doesn't work (well). When I have a defaultLocale and navigate to /privacy while my browser is set to another language (Dutch), i'm always redirected to the English version. |
@MikeScheurwater please report a separate issue with details (your configuration and ideally repo that reproduces). From your description, it doesn't seem like your issue is directly related. It looks like it's maybe a configuration issue. |
Hey @rchl,
It does have if the strategy is
Yes I fixed this
Yes this sounds like an option for this, thanks.
I meant the two websites I posted as a reference.
That's a good idea. There you go: |
i18n is not set up in that repo. |
@rchl No it's a prototype for the concept of this PR. What have you expected? |
I'm not sure what I've expected as I've lost the track of the conversation. :) |
I've pushed a change that puts this functionality behind an option and added some extra tests. It's good on my side, feel free to check it out before I merge it. |
Jup works for me. Thanks for your time and merging! 😊 |
Currently, the browser detection overrides an existing locale in the route. For example when I'm calling
/en/foo
, it will redirect to/de/foo
although there is already a locale in the route. This PR adjusts the priority./foo
->/de/foo
/de/foo
->/de/foo
/en/foo
->/en/foo