[wrangler] fix: throw helpful error if email validation required #4592

mrbbot · 2023-12-12T16:51:42Z

What this PR solves / how to test:

Previously, Wrangler would display the raw API error message and code if email validation was required during wrangler deploy. This change ensures a helpful error message is displayed instead, prompting users to check their emails or visit the dashboard for a verification link. To test this, try run wrangler deploy using a workers.dev subdomain on an account without a verified email. (I assume you've got one of those lying around! 😛)

Author has addressed the following:

Tests
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- Included
- Not necessary because:
Associated docs
- Issue(s)/PR(s):
- Not necessary because: just changing the error message for existing behaviour

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-12-12T16:51:47Z

🦋 Changeset detected

Latest commit: 62364fe

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 · 2023-12-12T16:54:01Z

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/7387147635/npm-package-wrangler-4592

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

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7387147635/npm-package-wrangler-4592

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7387147635/npm-package-wrangler-4592 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7387147635/npm-package-miniflare-4592

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7387147635/npm-package-cloudflare-pages-shared-4592

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7387147635/npm-package-create-cloudflare-4592 --no-auto-update

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.22.1 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20231030.4
`workerd`	1.20231030.0	1.20231030.0
`workerd --version`	1.20231030.0	2023-10-30

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2023-12-12T16:57:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ba9b88f) 75.53% compared to head (62364fe) 75.21%.
Report is 37 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4592      +/-   ##
==========================================
- Coverage   75.53%   75.21%   -0.33%     
==========================================
  Files         242      243       +1     
  Lines       12918    13031     +113     
  Branches     3324     3348      +24     
==========================================
+ Hits         9758     9801      +43     
- Misses       3160     3230      +70

Files	Coverage Δ
packages/wrangler/src/cfetch/errors.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/cfetch/index.ts	`90.47% <100.00%> (+0.18%)`	⬆️

... and 20 files with indirect coverage changes

dario-piotrowicz · 2023-12-12T17:29:21Z

I don't think this fully fixes #4187

As I mentioned in #4187 (comment), I think that a significant part of the issue is that wrangler asks the user various different questions regarding what subdomain they want to use, but all those questions are a bit pointless since at the end the command is still going to fail if the email is not verified

So while having a more proper error message is an improvement I still think that the email should also be verified upfront before the rest of the questions.

Would that be possible?

Note

I can't remember for sure if the subdomain gets generated.... it might or might not 😅

RamIdeas · 2023-12-19T12:09:39Z

packages/wrangler/src/__tests__/deploy.test.ts

+			await expect(
+				runWrangler("deploy ./index")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`"Please verify your account's email address and try again."`


Should this snapshot contain the 2nd line? "Check your email for a verification link, or login to https://dash.cloudflare.com and request a new one."

The additional text is added as a "note" to the error. This error snapshot matcher only considers the "message" of the error.

If we care about the second line then I think we would need to catch the error and then stringify it before checking it against a snapshot.

Updated the assertion to check notes too. 👍

petebacondarwin · 2023-12-21T17:05:40Z

packages/wrangler/src/__tests__/deploy.test.ts

+			await expect(
+				runWrangler("deploy ./index")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`"Please verify your account's email address and try again."`


The additional text is added as a "note" to the error. This error snapshot matcher only considers the "message" of the error.

petebacondarwin · 2023-12-21T17:06:23Z

packages/wrangler/src/__tests__/deploy.test.ts

+			await expect(
+				runWrangler("deploy ./index")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`"Please verify your account's email address and try again."`


If we care about the second line then I think we would need to catch the error and then stringify it before checking it against a snapshot.

petebacondarwin · 2023-12-21T17:07:34Z

packages/wrangler/src/cfetch/errors.ts

+}
+
+export function maybeThrowFriendlyError(error: FetchError) {
+	if (error.message === "workers.api.error.email_verification_required") {


Is the idea that this helper will be expanded to handle converting other API errors into helpful errors?

Yep, exactly. Seemed likely there are more errors that could be friendlier.

petebacondarwin · 2023-12-22T10:35:58Z

I don't think this fully fixes #4187

As I mentioned in #4187 (comment), I think that a significant part of the issue is that wrangler asks the user various different questions regarding what subdomain they want to use, but all those questions are a bit pointless since at the end the command is still going to fail if the email is not verified

So while having a more proper error message is an improvement I still think that the email should also be verified upfront before the rest of the questions.

Would that be possible?

Note

I can't remember for sure if the subdomain gets generated.... it might or might not 😅

Sadly, I think this is not really possible without changes to the backend that Wrangler talks to.

The order of requests is that we only get the email-verification error when we deploy to the subdomain, but we need to register the subdomain before we can deploy to it. So we need to ask the questions of the user before we can find out that they haven't validated their email.

I propose that we go ahead with this fix for now and decide whether we need to go further and change the flow somehow.

mrbbot requested a review from a team as a code owner December 12, 2023 16:51

mrbbot changed the title ~~fix: throw helpful error if email validation required~~ [wrangler] fix: throw helpful error if email validation required Dec 12, 2023

mrbbot force-pushed the bcoll/helpful-email-validation-error branch from 83bace9 to 332b484 Compare December 12, 2023 17:33

RamIdeas reviewed Dec 19, 2023

View reviewed changes

petebacondarwin approved these changes Dec 21, 2023

View reviewed changes

petebacondarwin added the error-messaging Improving user facing error messages label Dec 22, 2023

fix: throw helpful error if email validation required

62364fe

mrbbot force-pushed the bcoll/helpful-email-validation-error branch from 332b484 to 62364fe Compare January 2, 2024 14:52

mrbbot merged commit 20da658 into main Jan 2, 2024
17 checks passed

mrbbot deleted the bcoll/helpful-email-validation-error branch January 2, 2024 15:09

workers-devprod mentioned this pull request Jan 2, 2024

Version Packages #4655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wrangler] fix: throw helpful error if email validation required #4592

[wrangler] fix: throw helpful error if email validation required #4592

mrbbot commented Dec 12, 2023

changeset-bot bot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

dario-piotrowicz commented Dec 12, 2023 •

edited

Loading

RamIdeas Dec 19, 2023 •

edited

Loading

petebacondarwin Dec 21, 2023

petebacondarwin Dec 21, 2023

mrbbot Jan 2, 2024

petebacondarwin Dec 21, 2023

petebacondarwin Dec 21, 2023

petebacondarwin Dec 21, 2023

mrbbot Jan 2, 2024

petebacondarwin commented Dec 22, 2023

[wrangler] fix: throw helpful error if email validation required #4592

[wrangler] fix: throw helpful error if email validation required #4592

Conversation

mrbbot commented Dec 12, 2023

changeset-bot bot commented Dec 12, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 12, 2023 • edited Loading

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

dario-piotrowicz commented Dec 12, 2023 • edited Loading

RamIdeas Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

mrbbot Jan 2, 2024

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

petebacondarwin Dec 21, 2023

Choose a reason for hiding this comment

mrbbot Jan 2, 2024

Choose a reason for hiding this comment

petebacondarwin commented Dec 22, 2023

changeset-bot bot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

dario-piotrowicz commented Dec 12, 2023 •

edited

Loading

RamIdeas Dec 19, 2023 •

edited

Loading