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

fix: toggle workers.dev subdomains only when required #836

Merged
merged 1 commit into from
Apr 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .changeset/dirty-pandas-bake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"wrangler": patch
---

fix: toggle `workers.dev` subdomains only when required

This fix -

- passes the correct query param to check whether a workers.dev subdomain has already been published/enabled
- thus enabling it only when it's not been enabled
- it also disables it only when it's explicitly knows it's already been enabled

The effect of this is that publishes are much faster.
42 changes: 40 additions & 2 deletions packages/wrangler/src/__tests__/publish.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1584,7 +1584,26 @@ export default{
workers_dev: true,
});
writeWorkerSource();
mockUploadWorkerRequest();
mockUploadWorkerRequest({ available_on_subdomain: false });
mockSubDomainRequest();
mockUpdateWorkerRequest({ enabled: true });

await runWrangler("publish ./index");

expect(std.out).toMatchInlineSnapshot(`
"Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should not try to enable the workers.dev domain if it has been enabled before", async () => {
writeWranglerToml({
workers_dev: true,
});
writeWorkerSource();
mockUploadWorkerRequest({ available_on_subdomain: true });
mockSubDomainRequest();

await runWrangler("publish ./index");
Expand Down Expand Up @@ -1614,6 +1633,25 @@ export default{
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should not try to disable the workers.dev domain if it is not already available", async () => {
writeWranglerToml({
workers_dev: false,
});
writeWorkerSource();
mockSubDomainRequest("test-sub-domain", false);
mockUploadWorkerRequest({ available_on_subdomain: false });

// note the lack of a mock for the subdomain disable request

await runWrangler("publish ./index");

expect(std.out).toMatchInlineSnapshot(`
"Uploaded test-name (TIMINGS)
No publish targets for test-name (TIMINGS)"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("should disable the workers.dev domain if workers_dev is undefined but overwritten to `false` in environment", async () => {
writeWranglerToml({
env: {
Expand Down Expand Up @@ -3955,7 +3993,7 @@ function mockUploadWorkerRequest(
if (!legacyEnv) {
expect(envName).toEqual(env);
}
expect(queryParams.get("available_on_subdomain")).toEqual("true");
expect(queryParams.get("include_subdomain_availability")).toEqual("true");
const formBody = body as FormData;
if (expectedEntry !== undefined) {
expect(await (formBody.get("index.js") as File).text()).toMatch(
Expand Down
22 changes: 13 additions & 9 deletions packages/wrangler/src/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,15 @@ export default async function publish(props: Props): Promise<void> {
: `/accounts/${accountId}/workers/scripts/${scriptName}`;

// Upload the script so it has time to propagate.
const { available_on_subdomain } = await fetchResult(
const { available_on_subdomain } = await fetchResult<{
available_on_subdomain: boolean;
}>(
workerUrl,
{
method: "PUT",
body: createWorkerUploadForm(worker),
},
new URLSearchParams({ available_on_subdomain: "true" })
new URLSearchParams({ include_subdomain_availability: "true" })
Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that this is the correct parameter (by looking at the internal code). But it is worrying that this does not appear in our API docs, as far as I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a documented api, I found it on an internal wiki, I don't think it ever went public.

);

const uploadMs = Date.now() - start;
Expand Down Expand Up @@ -291,13 +293,15 @@ export default async function publish(props: Props): Promise<void> {
}
} else {
// Disable the workers.dev deployment
await fetchResult(`${workerUrl}/subdomain`, {
method: "POST",
body: JSON.stringify({ enabled: false }),
headers: {
"Content-Type": "application/json",
},
});
if (available_on_subdomain) {
await fetchResult(`${workerUrl}/subdomain`, {
method: "POST",
body: JSON.stringify({ enabled: false }),
headers: {
"Content-Type": "application/json",
},
});
}
}

console.log("Uploaded", workerName, formatTime(uploadMs));
Expand Down