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: cache account id selection #1395

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Jul 3, 2022

This adds caching for account id fetch/selection for all wrangler commands.

Currently, if we have an api/oauth token, but haven't provided an account id, we fetch account information from cloudflare. If a user has just one account id, we automatically choose that. If there are more than one, then we show a dropdown and ask the user to pick one. This is convenient, and lets the user not have to specify their account id when starting a project.

However, if does make startup slow, since it has to do that fetch every time. It's also annoying for folks with multiple account ids because they have to pick their account id every time.

So we now cache the account details into node_modules/.cache/wrangler (much like pages already does with account id and project name).

This patch also refactors config-cache.ts; it only caches if there's a node_modules folder, and it looks for the closest node_modules folder (and not directly in cwd). I also added tests for when a node_modules folder isn't available. It also trims the message that we log to terminal.

Closes #300


@GregBrimble Two changes in behaviour here for pages:

  • this will not cache account selection in projects without a node_modules folder. We can fix that by working on 🚀 Feature Request: Caching previous interactive choices #1318
  • I think this will log "Retrieving cached values..." twice. It's not too intrusive, but we could fix it by not caching the account_id in pages code at all (since wrangler will do it anyway).

Happy to iterate on this a bit, putting it out there for discussion. Let's chat about it during the week.

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2022

🦋 Changeset detected

Latest commit: 9b7e3b9

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 Jul 3, 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/2615545717/npm-package-wrangler-1395

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

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

Or you can use npx with this latest build directly:

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

@threepointone
Copy link
Contributor Author

Checked with @GregBrimble, the behaviour looks good on that side. Opening this up for review.

@threepointone threepointone marked this pull request as ready for review July 4, 2022 10:06
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

From what I can see we only list the keys of the cache file, which means that the user is going to see something like:

Retrieving cached values for account_id from some/path

As a user I think I would prefer to know which account was being used. Do we show that anywhere? Could we consider showing that? Perhaps even store both the account ID and name (if available)?

@threepointone
Copy link
Contributor Author

Could do! I tried that and it involved a refactor, and I didn't want to expand the scope beyond this PR. Would you be ok if I filed an issue and did it in a subsequent PR?

@petebacondarwin
Copy link
Contributor

Could do! I tried that and it involved a refactor, and I didn't want to expand the scope beyond this PR. Would you be ok if I filed an issue and did it in a subsequent PR?

Landing this and then making that change is likely to require a change to the data that is stored in the cache file, which would add complication to the follow up PR. If this idea is reasonable I would rather we get it right first time.

@threepointone
Copy link
Contributor Author

Fair feedback. I updated the PR to store id/name combo in the cache. In a subsequent PR I'll figure out a better way to log the details to the terminal.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Love it! Thanks for the update.
A couple of nits and questions

packages/wrangler/src/config-cache.ts Show resolved Hide resolved
packages/wrangler/src/config-cache.ts Show resolved Hide resolved
packages/wrangler/src/user/user.tsx Outdated Show resolved Hide resolved
This adds caching for account id fetch/selection for all wrangler commands.

Currently, if we have an api/oauth token, but haven't provided an account id, we fetch account information from cloudflare. If a user has just one account id, we automatically choose that. If there are more than one, then we show a dropdown and ask the user to pick one. This is convenient, and lets the user not have to specify their account id when starting a project.

However, if does make startup slow, since it has to do that fetch every time. It's also annoying for folks with multiple account ids because they have to pick their account id every time.

So we now cache the account details into `node_modules/.cache/wrangler` (much like pages already does with account id and project name).

This patch also refactors `config-cache.ts`; it only caches if there's a `node_modules` folder, and it looks for the closest node_modules folder (and not directly in cwd). I also added tests for when a `node_modules` folder isn't available. It also trims the message that we log to terminal.

Closes #300
@threepointone threepointone merged commit 88f2702 into main Jul 5, 2022
@threepointone threepointone deleted the cache-account-selection branch July 5, 2022 10:43
@github-actions github-actions bot mentioned this pull request Jul 5, 2022
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.

cache some calls in node_modules/.cache (specifically - account selection, but maybe more)
2 participants