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: optionally send zone_id with a route #778

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Apr 10, 2022

This enables optionally passing a route as {pattern: string, zone_id: string}. There are scenarios where we need to explicitly pass a zone_id to the api, so this is enables that.

Some nuance: The errors from the api aren't super useful when invalid values are passed, but that's something to further work on.

This also fixes some types in our cli parsing.

Fixes #774

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2022

🦋 Changeset detected

Latest commit: 8015ff3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2022

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/2154535104/npm-package-wrangler-778

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/778/npm-package-wrangler-778

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2154535104/npm-package-wrangler-778 dev path/to/script.js

threepointone added a commit that referenced this pull request Apr 10, 2022
This updates some dependencies. Some highlights -

- updates to `@iarna/toml` means we can have mixed types for inline arrays, which is great for #774 / #778
- I also moved timeago.js to `devDependencies` since it already gets compiled into the bundle
- updates to `esbuild` brings along a number of smaller fixes for modern js
threepointone added a commit that referenced this pull request Apr 10, 2022
This updates some dependencies. Some highlights -

- updates to `@iarna/toml` means we can have mixed types for inline arrays, which is great for #774 / #778
- I also moved timeago.js to `devDependencies` since it already gets compiled into the bundle
- updates to `esbuild` brings along a number of smaller fixes for modern js
petebacondarwin pushed a commit that referenced this pull request Apr 11, 2022
This updates some dependencies. Some highlights -

- updates to `@iarna/toml` means we can have mixed types for inline arrays, which is great for #774 / #778
- I also moved timeago.js to `devDependencies` since it already gets compiled into the bundle
- updates to `esbuild` brings along a number of smaller fixes for modern js
@github-actions github-actions bot mentioned this pull request Apr 11, 2022
@threepointone
Copy link
Contributor Author

aight, I think this is ready for a review/land. I'll work on adding zone_name afterwards.

@@ -327,15 +337,13 @@ export default async function publish(props: Props): Promise<void> {
if (triggers && triggers.length) {
deployments.push(
fetchResult(`${workerUrl}/schedules`, {
// TODO: Unlike routes, this endpoint does not support PATCH.
// So technically, this will override any previous schedules.
// We should change the endpoint to support PATCH.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this in the works, or no longer doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using patch is weird because it means the source of truth is now coming from two places. But of course it means you can edit it in the dashboard. Each option has tradeoffs, but this is the one we're picking for now and can revisit later based on feedback.

@WalshyDev
Copy link
Member

WalshyDev commented Apr 11, 2022

Can confirm it works 🎉

Thanks so much for the quick work!

$ npx https://prerelease-registry.developers.workers.dev/runs/2150140028/npm-package-wrangler-778 publish
 ⛅️ wrangler 0.0.0-72b5599 (update available 0.0.25)
--------------------------------------------------------------
Uploaded build-cache-poc (2.58 sec)
Published build-cache-poc (3.38 sec)
  pages-cache-poc.walshy.dev/* (zone: 8d0c8239f88f98a8cb82ec7bb29b8556)

(now I can start to get working on this demo I'm giving in a few days...)

@threepointone
Copy link
Contributor Author

Wonderful, good to hear

.changeset/dull-jobs-return.md Outdated Show resolved Hide resolved
packages/wrangler/src/config/validation.ts Show resolved Hide resolved
packages/wrangler/src/index.tsx Show resolved Hide resolved
This enables optionally passing a route as `{pattern: string, zone_id: string}`. There are scenarios where we need to explicitly pass a zone_id to the api, so this enables that.

Some nuance: our toml parser unfortunately doesn't let you have mixed types in arrays, so if you have a single route with this special shape, then you'll have to do it for all of them. We can replace it with something more recent, but I'll do that in a subsequent PR.

Further nuance: The errors from the api aren't super useful when invalid values are passed, but that's something to further work on.

This also fixes some types in our cli parsing.

Fixes #774
@threepointone threepointone merged commit 85b0c31 into main Apr 12, 2022
@threepointone threepointone deleted the route-zone-id branch April 12, 2022 11:18
threepointone added a commit that referenced this pull request Apr 14, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).
threepointone added a commit that referenced this pull request Apr 14, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
threepointone added a commit that referenced this pull request Apr 14, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
threepointone added a commit that referenced this pull request Apr 15, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
threepointone added a commit that referenced this pull request Apr 15, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
threepointone added a commit that referenced this pull request Apr 15, 2022
A followup to #778, this lets you send an optional `zone_name` with routes. This is particularly useful when using ssl for saas (https://developers.cloudflare.com/ssl/ssl-for-saas/).

Fixes #793
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.

feat: routes should be able to specify zone_id
4 participants