-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 support #9143
Conversation
🦋 Changeset detectedLatest commit: 666f3e3 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
f2099af
to
a749caa
Compare
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.
Did a quick review of the changeset first! (Have not yet looked at the API docs) because I want to make sure I understand what's happening here to get the API descriptions just right!
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 think it's better to use the 1 arg form of new URL
unless there's a reason to do it the other way that I can't think of?
a1c4cbc
to
4d7f6ed
Compare
4d7f6ed
to
6b4c762
Compare
Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
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.
We're going to release this in 4.x
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 your help with this one, @ematipico ! I fixed some silly formatting/typos in the changeset, but I think it's now quite good!
Had a suggestion for each of the config items. They might be a bit verbose, but we can always refine them, and once we've finalized the guide, we'll know for sure which info we want there vs here. I think these are still the minimum we might want someone to see when they're looking things up in reference, though? Let's see what we think!
fc65885
to
8b63b04
Compare
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.
OK! These update to the agreed-upon configuration set up! I'm happy with the changeset as is, but still want to fine-tune the docs entry, especially because of the experimental-ness of it. (This entry doesn't explicitly mention the experimental flag, for example and we must do that!
I'm thinking that maybe we should put the docs entry under Experimental Flags. I think that makes the most organizational sense, and would make the instruction to enable a flag, then add a domains
to a pre-configured i18n site easier to include here. Being experimental, this feature might change anyway. And even if it doesn't that means editing more than just the name of this entry to update config-reference here. So I'm not convinced we save much by having it here vs experimental.
Another option could be to write this entry here as if it were NOT experimental, just not expose it to docs. Then, add a complete corresponding entry as if it IS expermental in the Experimental flags section that DOES show up in docs. When it's no longer experimental, we entirely delete the experimental entry and (making sure it's updated), then add back @docs
here. (If I'm writing this once, copy and paste is free!)
Thoughts?
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
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.
ALRIGHT! I am approving for the sake of completion! 🥳
If there are any syntax errors or nits, I'll find them after this is merged on Wed. and I can check and make any quick fixes well before releasing on Thursday. 🫡
Merge day
I’m getting errors, despite not using
|
Changes
Closes PLT-1317
This PR adds domain support for i18n routing.
Here's the relevant RFC: withastro/roadmap#798
Technical changes
As stated in the RFC, there are a bunch of restrictions when enabling this functionality:
base
must be setoutput
must beserver
Hence, I added a bunch of errors and validation for the configuration schema
Theoretically, the Node.js adapter is the only one that would require less work to set up domains. Some work is still needed because the reserve proxy of the application needs to send the proper headers to the Node.js server, as explained in the RFC. Regardless, I preferred to mark the support as
experimental
.Regarding the Vercel adapter, I decided to mark it unsupported for now, although I am open to considering "experimental" too. I want to highlight that serverless environments require more work to set multiple domains, and I am not sure if we were able to set them up from thevercel.json
; for this reason I am not sure how to communicate it, and that's why I setunsupported
.Update: myself and @matthewp decided to flag the support as
experimental
.Testing
I added new tests that should verify the new behaviour of the
*Absolute
functions.I added new integrations tests to very that the
Request
s are correctly handled when we send the proper headers.Docs
/cc @withastro/maintainers-docs for feedback!