-
Notifications
You must be signed in to change notification settings - Fork 778
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
[wrangler] Output complete URL when deploying custom domains #8001
base: main
Are you sure you want to change the base?
Conversation
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-wrangler-8001 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8001/npm-package-wrangler-8001 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-wrangler-8001 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-workers-bindings-extension-8001 -O ./cloudflare-workers-bindings-extension.0.0.0-v10f35b6bd.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v10f35b6bd.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-create-cloudflare-8001 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-kv-asset-handler-8001 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-miniflare-8001 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-pages-shared-8001 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-unenv-preset-8001 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-vite-plugin-8001 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-vitest-pool-workers-8001 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-workers-editor-shared-8001 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-workers-shared-8001 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13119829352/npm-package-cloudflare-workflows-shared-8001 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
result = new URL(route, "https://").href; | ||
} else { | ||
result = route.pattern; | ||
result = new URL(route.pattern, "https://").href; |
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.
Calling new URL()
won't work here—these routes are not necessarily valid URLs, and can have wildcard patterns. I think this might have to resort to prepending the string https://
to custom domains?
e.g. "*.bar.com/foo/*"
is a perfectly valid route, but new URL()
won't parse it.
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.
Gah. I was trying to do the "right" thing. The first version of this code just appended the protocol. Will go back to that
deade44
to
6eb9e99
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.
I propose prepending only if it doesn't already start with http
@@ -238,11 +238,11 @@ export const validateRoutes = (routes: Route[], assets?: AssetsOptions) => { | |||
}; | |||
|
|||
export function renderRoute(route: Route): string { | |||
let result = ""; | |||
let result = "https://"; |
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.
Unfortunately you will need to accommodate this feature: https://developers.cloudflare.com/workers/configuration/routing/routes/#:~:text=Route%20patterns%20may%20optionally%20begin%20with%20http%3A//%20or%20https%3A//
I'm realizing there is a bug behind the bug:
If you give a fully specified protocol in the custom domain it errors on that line. But the code there is only checking for paths |
The current CLI output for custom domains does not include the URL. In iterm2 (and probably other terminals) this means the link cannot be clicked. Adding the full URL to the output makes it easier to jump to the deployed URL with a single click.