Add support for publishing to Custom domains #982
Conversation
matthewdavidrodgers
requested review from
threepointone and
petebacondarwin
as code owners
🦋 Changeset detectedLatest commit: 6a8f041 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| function getQuoteBoundedSubstring(content: string) { | ||
| const matches = content.split('"'); | ||
| return matches[1] ?? ""; | ||
| } |
There was a problem hiding this comment.
Originally tried to use a regex here, but it was too permissive when there were multiple quote bounded substrings. This will always grab the first one (and even works when the string begins with a quote)
There was a problem hiding this comment.
Is there any time when the substring might contain an escaped quote?
There was a problem hiding this comment.
there's no current situation where this could occur - I authored the code that generates these messages - but I suppose all bets are off if the code drifts. I could add a comment reinforcing the general structure in the api
There was a problem hiding this comment.
Maybe @threepointone would have a good opinion on this, but I wonder if it makes sense for this feature to be separate from routes, something like:
domains = ["api.example.com"]The reason I propose this is because I think we want to push folks to use domains instead of routes, since it sets up DNS, SSL, and all that jazz. If we jam them together, it might get confusing.
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2342037445/npm-package-wrangler-982You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/982/npm-package-wrangler-982Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2342037445/npm-package-wrangler-982 dev path/to/script.js |
|
Another option is to use Thanks a lot for the PR @matthewdavidrodgers! This looks extensive, and thank you for adding tests! We'll review this soon. |
There was a problem hiding this comment.
Love this PR. I honestly don't have any notes, but I'll let @petebacondarwin have a quick look at this before I stamp it. Could you also rebase to head? We've landed a few changes since this came in.
There was a problem hiding this comment.
Also love this PR. Made a few comments but none blocking merging this.
Thanks @matthewdavidrodgers.
| function getQuoteBoundedSubstring(content: string) { | ||
| const matches = content.split('"'); | ||
| return matches[1] ?? ""; | ||
| } |
There was a problem hiding this comment.
Is there any time when the substring might contain an escaped quote?
| // this is useful because the /domains api will return | ||
| // which domains conflicted in an error message, bounded | ||
| // by a string, which we can use to provide helpful | ||
| // messages to a user |
There was a problem hiding this comment.
Would it be better to change the CF API so that it returned a structured response that we could parse more reliably?
There was a problem hiding this comment.
so, went to the api team today to try and see what ways we can provide structured data in an error response, and there weren't a whole lot of options they felt would work. something simple and trivially parseable within the error message seems to be the best way to handle this.
currently, the error messages look something like this:
Cannot create Custom Domain "<domain>": Custom Domain already exists and points to a different script; retry and use option "override_existing_origin" to override
which is why I parse out the first quote bounded substring.
do we feel better if i change the error message to look something like this:
"<domain>" could not be created as a Custom Domain; already exists and points to a different script; retry and use option "override_existing_origin" to override
so at least the conflicting domains are the first thing in the message? maybe making the parsing less fraught?
|
I think @threepointone’s idea of |
I went with the custom_domain boolean because it's likely we will have support for something like wildcard operators in custom domains in the future, and "pattern" will actually fit well. As for the point around a separate top level key in the toml (i.e. |
Needed to re-write a "renderer" of the route to be logged out to the user, but it's now supported in the type system
If a toml provides routes with the "custom_domain" flag, publish those separately from normal routes, using the /domains api. As these are more complex than simple routes, we ask the api to error eagerly. If it errors on a conflict for existing custom domains or managed DNS records, we prompt for confirmation to override the conflicts, and then retry with updated parameters
matthewdavidrodgers
force-pushed
the
custom-domains
branch
from
33c048b
to
6a8f041
Compare
With the release of Custom Domains for workers, users can publish directly to a custom domain on a route, rather than creating a dummy DNS record first and manually pointing the worker over - this adds the same support to wrangler.
Users declare routes as normal, but to indicate that a route should be treated as a custom domain, a user simply uses the object format in the toml file, but with a new key: custom_domain (i.e.
routes = [{ pattern = "api.example.com", custom_domain = true }])When wrangler sees a route like this, it peels them off from the rest of the routes and publishes them separately, using the /domains api. This api is very defensive, erroring eagerly if there are conflicts in existing Custom Domains or managed DNS records. In the case of conflicts, wrangler prompts for confirmation, and then retries with parameters to indicate overriding is allowed.