-
Notifications
You must be signed in to change notification settings - Fork 30
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
rfc: i18n routing domain support #798
Conversation
9488aab
to
8198349
Compare
In addition to the inline comments, one question I have is whether you have considered it makes sense to support domains and path prefixes at the same time. For example you might have domains for your most common locales but not for all of them. I'm not trying to blow up the scope, but would that be difficult to support (if it's even a good idea)? cc @delucis |
If the users use the utilities that we provide to create the URLs, we won't have problems with that. |
Matthew perfectly covered all the questions I had, so looking forward to the answers to his comments! Looks great, I'm excited to see this land in Astro, our i18n support is really starting to look quite complete, it's an awesome story. |
So if this is supported, would that still fall under the |
The commit |
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.
Saw the PTAL @ematipico, so left a few comments/questions! 🙌
``` | ||
|
||
The following APIs will behave as follows: | ||
- [`getRelativeLocaleUrl`](#getrelativelocaleurllocale-string-string): it won't prefix the locale to the URL. From `/en` to `/`; |
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.
Wondering if this should even be an error or something in this case? Seems like kind of a footgun if you use getRelativeLocaleUrl()
hoping to navigate to a different locale, but get a same-domain random path instead.
Also to make sure I follow this. With the example config above, would you get the following behaviour?
getRelativeLocaleUrl('en', '') // => '/en'
getRelativeLocaleUrl('es', '') // => '/es'
getRelativeLocaleUrl('fr', '') // => '/'
And is that consistent across domains? i.e. for a page on fr.example.com
, does getRelativeLocaleUrl('en', '')
still return '/en'
? That would resolve to fr.example.com/en
, which seems kind of useless.
Should sites using multiple domains perhaps always use getAbsoluteLocaleUrl()
? At the most extreme end, we could even change the type of astro:i18n
and remove getRelativeLocaleUrl()
entirely in these cases.
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.
And is that consistent across domains? i.e. for a page on fr.example.com, does getRelativeLocaleUrl('en', '') still return '/en'? That would resolve to fr.example.com/en, which seems kind of useless.
No, it's not consistent and it's impossible to make it consistent.
Should sites using multiple domains perhaps always use getAbsoluteLocaleUrl()? At the most extreme end, we could even change the type of astro:i18n and remove getRelativeLocaleUrl() entirely in these cases.
Is it something that we can do? 😮 If so, I think we should go this way, although it should make the transition from a non-domain website to a domain website a horrible experience, because a user would have to change all APIs in one go in order to make it work.
Maybe we should do something in the middle; if the user uses a getRelativeLocale*
function, we should fall back to use getAbsoluteLocale*
instead. And we could add a warning.
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.
Also, I realised that getRelativeLocaleUrl
should not change its behaviour. I removed that line
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.
Maybe we should do something in the middle; if the user uses a
getRelativeLocale*
function, we should fall back to usegetAbsoluteLocale*
instead. And we could add a warning.
Yes, that would be a nice compromise I think and wouldn’t need the type to change (I assume? The two functions have the same signature right?) while still giving feedback that the function used is not intended for this context.
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.
I'm conceptually onboard with all of this! Hopefully we can lift the prerendering restriction at some point, but I know that's a whole other topic. This is definitely #NWTWWHB!
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.
Thanks for addressing my comments @ematipico! Sounds good to me.
Posting one more bit of feedback that I already shared on Discord.
FYI, we won't need a new |
Call for consensus starts now |
The final comment period has ended and this RFC is accepted! It can now be unflagged in Astro. |
Rendered RFC