fix: do not crash in wrangler dev if user has multiple accounts

petebacondarwin · petebacondarwin · commit 10255eaf5e5e · 2022-06-20T14:37:32.000+01:00
When a user has multiple accounts we show a prompt to allow the user to select which they should use. This was broken in wrangler dev as we were trying to start a new ink.js app (to show the prompt) from inside a running ink.js app (the UI for wrangler dev). This fix refactors the ChooseAccount component so that it can be used directly within another component. Fixes #1258
diff --git a/.changeset/proud-cougars-sell.md b/.changeset/proud-cougars-sell.md
@@ -0,0 +1,13 @@
+---
+"wrangler": patch
+---
+
+fix: do not crash in `wrangler dev` if user has multiple accounts
+
+When a user has multiple accounts we show a prompt to allow the user to select which they should use.
+This was broken in `wrangler dev` as we were trying to start a new ink.js app (to show the prompt)
+from inside a running ink.js app (the UI for `wrangler dev`).
+
+This fix refactors the `ChooseAccount` component so that it can be used directly within another component.
+
+Fixes #1258
diff --git a/packages/wrangler/src/dev/remote.tsx b/packages/wrangler/src/dev/remote.tsx
@@ -1,13 +1,14 @@
 import { readFile } from "node:fs/promises";
 import path from "node:path";
-import { useState, useEffect, useRef } from "react";
+import React, { useState, useEffect, useRef } from "react";
+import { useErrorHandler } from "react-error-boundary";
 import { printBundleSize } from "../bundle-reporter";
 import { createWorkerPreview } from "../create-worker-preview";
 import useInspector from "../inspect";
 import { logger } from "../logger";
 import { usePreviewServer } from "../proxy";
 import { syncAssets } from "../sites";
-import { requireApiToken, requireAuth } from "../user";
+import { ChooseAccount, requireApiToken } from "../user";
 import type { CfPreviewToken } from "../create-worker-preview";
 import type { AssetPaths } from "../sites";
 import type { CfModule, CfWorkerInit, CfScriptFormat } from "../worker";
@@ -33,12 +34,13 @@ export function Remote(props: {
   zone: string | undefined;
   host: string | undefined;
 }) {
+  const [accountId, setAccountId] = useState(props.accountId);
   const previewToken = useWorker({
     name: props.name,
     bundle: props.bundle,
     format: props.format,
     modules: props.bundle ? props.bundle.modules : [],
-    accountId: props.accountId,
+    accountId,
     bindings: props.bindings,
     assetPaths: props.assetPaths,
     isWorkersSite: props.isWorkersSite,
@@ -67,7 +69,16 @@ export function Remote(props: {
     port: props.inspectorPort,
     logToTerminal: true,
   });
-  return null;
+
+  const errorHandler = useErrorHandler();
+
+  return !accountId ? (
+    <ChooseAccount
+      isInteractive={true}
+      onSelect={(selectedAccountId) => setAccountId(selectedAccountId)}
+      onError={(err) => errorHandler(err)}
+    ></ChooseAccount>
+  ) : null;
 }
 
 export function useWorker(props: {
@@ -107,13 +118,13 @@ export function useWorker(props: {
   // something's "happened" in our system; We make a ref and
   // mark it once we log our initial message. Refs are vars!
   const startedRef = useRef(false);
-  // This ref holds the actual accountId being used, the `accountId` prop could be undefined
-  // as it is only what is retrieved from the wrangler.toml config.
-  const accountIdRef = useRef(accountId);
 
   useEffect(() => {
     const abortController = new AbortController();
     async function start() {
+      if (accountId === undefined) {
+        return;
+      }
       setToken(undefined); // reset token in case we're re-running
 
       if (!bundle || !format) return;
@@ -124,11 +135,6 @@ export function useWorker(props: {
         logger.log("⎔ Detected changes, restarted server.");
       }
 
-      // Ensure we have an account id, even if it means logging in here.
-      accountIdRef.current = await requireAuth({
-        account_id: accountIdRef.current,
-      });
-
       const content = await readFile(bundle.path, "utf-8");
 
       // TODO: For Dev we could show the reporter message in the interactive box.
@@ -138,7 +144,7 @@ export function useWorker(props: {
       );
 
       const assets = await syncAssets(
-        accountIdRef.current,
+        accountId,
         // When we're using the newer service environments, we wouldn't
         // have added the env name on to the script name. However, we must
         // include it in the kv namespace name regardless (since there's no
@@ -189,7 +195,7 @@ export function useWorker(props: {
         await createWorkerPreview(
           init,
           {
-            accountId: accountIdRef.current,
+            accountId,
             apiToken: requireApiToken(),
           },
           {
@@ -214,7 +220,7 @@ export function useWorker(props: {
             "Error: You need to register a workers.dev subdomain before running the dev command in remote mode";
           const solutionMessage =
             "You can either enable local mode by pressing l, or register a workers.dev subdomain here:";
-          const onboardingLink = `https://dash.cloudflare.com/${accountIdRef.current}/workers/onboarding`;
+          const onboardingLink = `https://dash.cloudflare.com/${accountId}/workers/onboarding`;
           logger.error(
             `${errorMessage}\n${solutionMessage}\n${onboardingLink}`
           );
diff --git a/packages/wrangler/src/user/choose-account.tsx b/packages/wrangler/src/user/choose-account.tsx
@@ -0,0 +1,76 @@
+import { Text } from "ink";
+import SelectInput from "ink-select-input";
+import React, { useEffect, useRef, useState } from "react";
+import { fetchListResult } from "../cfetch";
+import { logger } from "../logger";
+import { getCloudflareAccountIdFromEnv } from "./env-vars";
+
+export type ChooseAccountItem = {
+  id: string;
+  name: string;
+};
+
+export function ChooseAccount(props: {
+  isInteractive: boolean;
+  onSelect: (accountId: string) => void;
+  onError: (error: Error) => void;
+}) {
+  const [accounts, setAccounts] = useState<ChooseAccountItem[]>([]);
+  const getAccountsPromiseRef =
+    useRef<Promise<{ account: ChooseAccountItem }[]>>();
+
+  useEffect(() => {
+    async function selectAccount() {
+      const accountIdFromEnv = getCloudflareAccountIdFromEnv();
+      if (accountIdFromEnv) {
+        props.onSelect(accountIdFromEnv);
+      } else {
+        getAccountsPromiseRef.current ??= fetchListResult<{
+          account: ChooseAccountItem;
+        }>(`/memberships`);
+        const response = await getAccountsPromiseRef.current;
+        if (response.length === 0) {
+          props.onError(
+            new Error(
+              "Failed to automatically retrieve account IDs for the logged in user.\n" +
+                "In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as `account_id` in your `wrangler.toml` file."
+            )
+          );
+        } else if (response.length === 1) {
+          props.onSelect(response[0].account.id);
+        } else if (props.isInteractive) {
+          setAccounts(response.map((x) => x.account));
+        } else {
+          props.onError(
+            new Error(
+              "More than one account available but unable to select one in non-interactive mode.\n" +
+                `Please set the appropriate \`account_id\` in your \`wrangler.toml\` file.\n` +
+                `Available accounts are ("<name>" - "<id>"):\n` +
+                response
+                  .map((x) => `  "${x.account.name}" - "${x.account.id}")`)
+                  .join("\n")
+            )
+          );
+        }
+      }
+    }
+    selectAccount().catch((err) => props.onError(err));
+  }, [props]);
+
+  return accounts.length > 0 ? (
+    <>
+      <Text bold>Select an account from below:</Text>
+      <SelectInput
+        items={accounts.map((item) => ({
+          key: item.id,
+          label: item.name,
+          value: item,
+        }))}
+        onSelect={(item) => {
+          logger.log(`Using account: "${item.value.name} - ${item.value.id}"`);
+          props.onSelect(item.value.id);
+        }}
+      />
+    </>
+  ) : null;
+}
diff --git a/packages/wrangler/src/user/index.ts b/packages/wrangler/src/user/index.ts
@@ -1,2 +1,3 @@
 export * from "./user";
 export * from "./env-vars";
+export * from "./choose-account";
diff --git a/packages/wrangler/src/user/user.tsx b/packages/wrangler/src/user/user.tsx
@@ -215,23 +215,18 @@ import url from "node:url";
 import { TextEncoder } from "node:util";
 import TOML from "@iarna/toml";
 import { HostURL } from "@webcontainer/env";
-import { render, Text } from "ink";
-import SelectInput from "ink-select-input";
+import { render } from "ink";
 import Table from "ink-table";
 import React from "react";
 import { fetch } from "undici";
-import { fetchListResult } from "../cfetch";
 import { purgeConfigCaches } from "../config-cache";
 import { logger } from "../logger";
 import openInBrowser from "../open-in-browser";
 import { parseTOML, readFileSync } from "../parse";
-import {
-  getCloudflareAccountIdFromEnv,
-  getCloudflareAPITokenFromEnv,
-} from "./env-vars";
+import { ChooseAccount } from "./choose-account";
+import { getCloudflareAPITokenFromEnv } from "./env-vars";
 import { generateAuthUrl } from "./generate-auth-url";
 import { generateRandomState } from "./generate-random-state";
-import type { Item as SelectInputItem } from "ink-select-input/build/SelectInput";
 import type { ParsedUrlQuery } from "node:querystring";
 
 /**
@@ -1073,72 +1068,21 @@ export async function getAccountId(
 ): Promise<string | undefined> {
   const apiToken = getAPIToken();
   if (!apiToken) return;
-
-  const accountIdFromEnv = getCloudflareAccountIdFromEnv();
-  if (accountIdFromEnv) {
-    return accountIdFromEnv;
-  }
-
-  let accounts: { account: ChooseAccountItem }[] = [];
-  let accountId: string | undefined;
-  accounts = await fetchListResult<{
-    account: ChooseAccountItem;
-  }>(`/memberships`);
-  if (accounts.length === 0) {
-    throw new Error(
-      "Failed to automatically retrieve account IDs for the logged in user.\n" +
-        "In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as `account_id` in your `wrangler.toml` file."
-    );
-  }
-  if (accounts.length === 1) {
-    accountId = accounts[0].account.id;
-  } else if (isInteractive) {
-    accountId = await new Promise((resolve) => {
-      const accountIds = accounts.map((x) => x.account);
-      const { unmount } = render(
-        <ChooseAccount
-          accounts={accountIds}
-          onSelect={async (selected) => {
-            resolve(selected.value.id);
-            unmount();
-          }}
-        />
-      );
-    });
-  } else {
-    throw new Error(
-      "More than one account available but unable to select one in non-interactive mode.\n" +
-        `Please set the appropriate \`account_id\` in your \`wrangler.toml\` file.\n` +
-        `Available accounts are ("<name>" - "<id>"):\n` +
-        accounts
-          .map((x) => `  "${x.account.name}" - "${x.account.id}")`)
-          .join("\n")
-    );
-  }
-  return accountId;
-}
-
-type ChooseAccountItem = {
-  id: string;
-  name: string;
-};
-export function ChooseAccount(props: {
-  accounts: ChooseAccountItem[];
-  onSelect: (item: SelectInputItem<ChooseAccountItem>) => void;
-}) {
-  return (
-    <>
-      <Text bold>Select an account from below:</Text>
-      <SelectInput
-        items={props.accounts.map((item) => ({
-          key: item.id,
-          label: item.name,
-          value: item,
-        }))}
-        onSelect={props.onSelect}
+  return await new Promise((resolve, reject) => {
+    const { unmount } = render(
+      <ChooseAccount
+        isInteractive={isInteractive}
+        onSelect={async (selected) => {
+          resolve(selected);
+          unmount();
+        }}
+        onError={(err) => {
+          reject(err);
+          unmount();
+        }}
       />
-    </>
-  );
+    );
+  });
 }
 
 /**

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
`1`	`1`	`export * from "./user";`
`2`	`2`	`export * from "./env-vars";`
	`3`	`+export * from "./choose-account";`