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

stop login console spam #695

Merged
merged 9 commits into from
Mar 30, 2022
Merged

stop login console spam #695

merged 9 commits into from
Mar 30, 2022

Conversation

caass
Copy link
Contributor

@caass caass commented Mar 25, 2022

If a user hasn't logged in and then they run a command that needs a login they'll get bounced to the login flow.
The login flow (if completed) would write their shiny new OAuth2 credentials to disk, but wouldn't reload the
in-memory state. This led to issues like #693, where even though the user was logged in on-disk, wrangler
wouldn't be aware of it.

We now update the in-memory login state each time new credentials are written to disk.

@caass caass requested a review from petebacondarwin March 25, 2022 15:14
@changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2022

🦋 Changeset detected

Latest commit: 7aa4794

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 Mar 25, 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/2059265420/npm-package-wrangler-695

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

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

Or you can use npx with this latest build directly:

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

Cass Fridkin added 7 commits March 29, 2022 06:33
Partially this involves mocking `open-in-browser.ts`, which I've
chosen to mock out in `user.test.ts` rather than `jest.setup.ts` since
we might later want to mock it out differently for testing wrangler dev
@caass caass force-pushed the stop-login-console-spam branch from 3f94ed2 to 63b5148 Compare March 29, 2022 13:28
@caass caass marked this pull request as ready for review March 29, 2022 13:46
@caass caass requested a review from threepointone as a code owner March 29, 2022 13:46
@caass caass linked an issue Mar 29, 2022 that may be closed by this pull request
And just disable it for login checks
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.

NICE work @caass

@@ -963,6 +979,8 @@ export async function login(props?: LoginProps): Promise<boolean> {
server.listen(8976);
});

await openInBrowser(urlToOpen);
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: moving this here, helps to ensure that the HTTP server that will receive the callback from the OAuth provider is already listening before we try to open the OAuth web page.

@petebacondarwin petebacondarwin merged commit 48fa89b into main Mar 30, 2022
@petebacondarwin petebacondarwin deleted the stop-login-console-spam branch March 30, 2022 08:25
@github-actions github-actions bot mentioned this pull request Mar 30, 2022
@threepointone
Copy link
Contributor

Such a good PR.

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.

🐛 BUG: npm start after forced login results in a lot of warnings
3 participants