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

rfc: i18n routing domain support #798

Merged
merged 5 commits into from May 21, 2024
Merged

rfc: i18n routing domain support #798

merged 5 commits into from May 21, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Dec 21, 2023

@matthewp
Copy link
Contributor

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

@ematipico
Copy link
Member Author

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.

If the users use the utilities that we provide to create the URLs, we won't have problems with that.

@Princesseuh
Copy link
Member

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.

@matthewp
Copy link
Contributor

@ematipico

If the users use the utilities that we provide to create the URLs, we won't have problems with that.

So if this is supported, would that still fall under the routingStrategy: "domain"? If so could you maybe add this to the RFC and explain that it is supported.

@ematipico
Copy link
Member Author

The commit cc268a6 (#798) adds some restrictions and clarifications based on everybody's feedback

Copy link
Member

@delucis delucis left a 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! 🙌

proposals/0046-i18n-domain-support.md Outdated Show resolved Hide resolved
```

The following APIs will behave as follows:
- [`getRelativeLocaleUrl`](#getrelativelocaleurllocale-string-string): it won't prefix the locale to the URL. From `/en` to `/`;
Copy link
Member

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.

Copy link
Member Author

@ematipico ematipico Jan 26, 2024

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.

Copy link
Member Author

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

Copy link
Member

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 use getAbsoluteLocale* 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.

proposals/0046-i18n-domain-support.md Outdated Show resolved Hide resolved
Copy link
Member

@natemoo-re natemoo-re left a 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!

Copy link
Member

@delucis delucis left a 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.

proposals/0046-i18n-domain-support.md Outdated Show resolved Hide resolved
@ematipico
Copy link
Member Author

FYI, we won't need a new i18n.routing.strategy variant. I updated the implementation PR.

@ematipico
Copy link
Member Author

Call for consensus starts now

@matthewp
Copy link
Contributor

The final comment period has ended and this RFC is accepted! It can now be unflagged in Astro.

@matthewp matthewp merged commit 1dc1d3f into main May 21, 2024
@matthewp matthewp deleted the feat/i18n-domain-support branch May 21, 2024 12:48
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

6 participants