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

feat(i18n): domain with lookup table #9112

Merged
merged 7 commits into from
Nov 17, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Nov 15, 2023

Changes

Relative RFC change: withastro/roadmap@713fce7

This PR adds the basic logic for supporting the new routing strategy called domain. We require a new routing strategy because Astro needs to make assumptions** in order to provide the correct user experience.

This assumption is that the localised folders must be at the root - src/pages folder. With that, the logic works as follow: during the build phase, we add to the manifest a new information called domainLookupTable. This table is a map that will look like this:

const domainLookupTable = {
	"https://fr.example.com": "fr",
	"https://example.pt": "pt",
}

It's essentially the configuration of i18n.domains, reversed :)

This table maps what we are supposed to get from the server to the correct route. When we extract the correct route, we can load and render the component for that route.

In a server environment, Astro will work with the following headers:

If Astro can't crate a URL from those heasers, it will return a 404. I believe this is an important information and I will make sure to add it to the RFC later.

I will have to do some testing, even on a serverless environment and check if that information is available. I want to do more testing after merging this PR.

Closes PLT-1218

Testing

I added some basic testing to show the feature, although I want to add more tests in another PR.

Docs

/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Nov 15, 2023

⚠️ No Changeset found

Latest commit: 28f5a7c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 15, 2023
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a review comment, thinking that having a full lookup table for all routes won't work.

packages/astro/src/core/build/plugins/plugin-manifest.ts Outdated Show resolved Hide resolved
Comment on lines +1536 to +1538
* - `domain`: SSR only, it enables support for different domains. When a locale is mapped to domain, all the URLs won't have the language prefix.
* You map `fr` to `fr.example.com`, if you want a to have a blog page to look like `fr.example.com/blog` instead of `example.com/fr/blog`.
* The localised folders be must in the `src/pages/` folder.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions about how this works to be sure I understand:

  1. if you select routingStrategy: "domain", what happens to the URLs for your defaultLocale? Does it depend on whether you're using a /[defaultLocale]/ folder structure? Does it make any assumptions by default?
  2. Is it correct to assume that for non-default locales, which will all be written in /locale/ folders, that the URLs will be prefixed if you don't specifically add them to domains? So, it's possible for a site to have a mixture of e.g. example.com/fr/ and es.example.com?
  3. is domains ignored if routingStrategy is set to one of the two other options? Could it cause problems to have domains configured if you do NOT have domain set as your routing strategy?
  4. Does the order of domains and routingStrategy matter in the i18n configuration? Could someone run into trouble if these are not in the proper order?
  5. Looking at the example below, routingStrategy is outside of the object containing the other config options. In the docs, we show it as inside this object. Can you confirm which should be correct?

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah, there are assumptions. If you want to map a locale to a domain, the locale folder must be at src/pages/[locale]. The same goes for the defaultLocale. If the default locale isn't mapped to a domain, there aren't any assumptions. Although we are still evaluating the behaviour of defaultLocale, so it's possible we will change it soon.
  2. Yes, that is correct.
  3. No, that will be an error. For example, if you have i18n.domains and routingStategy: "prefix-always", Astro will throw a validation nerror.
  4. No, the order doesn't matter.
  5. Mmmmh, routingStategy is inside the i18n, same goes for domains. Are you referring to something else?

@sarah11918
Copy link
Member

I'm also pointing out that #9099 was merged without my seeing it, which becomes confusing because now those docs changes do not show up as new here.

@ematipico
Copy link
Member Author

ematipico commented Nov 16, 2023

I'm also pointing out that #9099 was merged without my seeing it, which becomes confusing because now those docs changes do not show up as new here.

Don't worry Sarah, I haven't requested your review for that PR on purpose. I will loop you in when I require your review. These PR aren't published yet on main.

@sarah11918
Copy link
Member

sarah11918 commented Nov 16, 2023

But you did request me on this one which has those changes in it already, so I'll just point out that it's very difficult for me to look at these individual pieces in isolation like this. These concepts don't exist independently from each other, and much of documentation is about how the whole picture works and the pieces fit together.

Should I not be looking at this one yet?

Edit: Sorry, I realized that "PTAL" is pretty broad, and maybe you weren't asking me to edit these yet, but maybe to give feedback or comment? As you saw, I couldn't really look at wording until it was clearer to me how things worked. Maybe PTAL in this case was actually Please Throw Your Questions At Me Now! I should have asked you to clarify what I was taking a look at/for and what would be helpful for you!

(I'm sorry, I just got confused seeing docs merged that I hadn't seen related to this, and didn't know that I wasn't supposed to pay attention to them, because we're always trying to document all the features in relation to each other. So if it's there, I'm going to check that it's all consistent!)

@ematipico ematipico removed the pr: docs A PR that includes documentation for review label Nov 17, 2023
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 17, 2023
@ematipico ematipico removed the pr: docs A PR that includes documentation for review label Nov 17, 2023
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 17, 2023
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ematipico ematipico merged commit fdb3464 into feat/i18n-domain Nov 17, 2023
14 checks passed
@ematipico ematipico deleted the feat/lookup-table-domain branch November 17, 2023 19:35
ematipico added a commit that referenced this pull request Nov 20, 2023
ematipico added a commit that referenced this pull request Nov 21, 2023
ematipico added a commit that referenced this pull request Jan 31, 2024
* i18n(domains): validation and updated logic (#9099)

* feat(i18n): domain with lookup table (#9112)

* chore: add changelog, fix types and enable experimental support in node/vercel

* rebase and update lock file

* chore: fix failing test

* Apply suggestions from code review

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by:  Matthew Phillips <matthew@skypack.dev>

* Update .changeset/tidy-carrots-jump.md

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* wip

* chore: rebase, conflicts and tests

* update lock file

* chore: correct configuration

* chore: correct configuration

* fix: regressions

* chore: fix conflicts and add more tests

* chore: add more validation

* chore: more tests and add more restrictions

* fix changeset

* change and revert adapters

* add another restriction

* lock file update

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

* wat

* fix syntax error

* fix config example

* Fix for #9673 (#9680)

* Fix for #9673

* 🦋 add changeset file

* Update breezy-plants-smoke.md

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* ⚡️ simplified normalizeConfigPath

---------

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* Fix env var replacement for export const prerender (#9807)

* feat(alpinejs): allow customizing the Alpine instance (#9751)

* feat(alpinejs): allows customzing the Alpine instance

* chore: add e2e tests

* fix: rename script

* Update index.ts

* fix: lockfile

* [ci] format

* chore: use correct lock file

* chore: rebase

* fix regressions in tests

* fix regressions in tests

* fix build

* add description

* fix missing types

* chore: fix tests, again :D

* eslint

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

* chore: address feedback

* chore: fix regressions

* chore: refactor naming

* Update packages/astro/src/core/app/index.ts

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

* chore: address feedback

* update lock file

* chore: infer routing from options, not strategy

* merge from main

* merge from main

* Experimental support in vercel adapter

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* Update .changeset/tidy-carrots-jump.md

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* better changesets

* Updates both experimental.i18nDomains and i18ndomains for experimental strategy

* fix link syntax

* consistent tabs/spaces

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* apply suggestion

---------

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Matthew Phillips <matthew@skypack.dev>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Lou Cyx <git@lou.cx>
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Co-authored-by: Florian Lefebvre <ematipico@users.noreply.github.com>
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants