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: disconnect extension when server is closed VSCODE-536 #734

Merged
merged 43 commits into from
Jun 26, 2024

Conversation

alenakhineika
Copy link
Contributor

@alenakhineika alenakhineika commented May 23, 2024

Description

Bump some minor dependencies and fix VSCODE-536.

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

src/connectionController.ts Outdated Show resolved Hide resolved
src/test/suite/oidc.test.ts Outdated Show resolved Hide resolved
@Anemy
Copy link
Member

Anemy commented Jun 3, 2024

Are we going to continue this pr re #737 ?

@alenakhineika
Copy link
Contributor Author

@Anemy you probably missed it during standup, yes this PR was in the debug mode therefore there are some prints around. I will clean this and ping you when ready to take a look.

@alenakhineika alenakhineika changed the title chore: bump minor dependencies fix: disconnect extension when server is closed VSCODE-536 Jun 4, 2024
@alenakhineika alenakhineika requested a review from Anemy June 25, 2024 14:02
@alenakhineika alenakhineika removed the WIP label Jun 25, 2024
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Nice, left a question/small comment or two

src/connectionController.ts Outdated Show resolved Hide resolved
src/connectionController.ts Show resolved Hide resolved
src/test/suite/connectionController.test.ts Outdated Show resolved Hide resolved
src/test/suite/oidc.test.ts Outdated Show resolved Hide resolved
@@ -398,6 +406,7 @@ suite('OIDC Tests', function () {
}

await reAuthPromise;
await sleep(100);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this sleep now? Can this be down without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to ensure here that it is enough time to capture the closing event. Let's try without sleeps, and see how tests react.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we do need sleep here otherwise the connection Is not yet closed, but I cleaned up some other places that you have mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of sleep we can use something like a waitFor with a timeout that keeps increasing, that way we will complete in 5ms if the end condition is already met.

src/views/webview-app/use-connection-form.ts Show resolved Hide resolved
@alenakhineika alenakhineika requested a review from Anemy June 25, 2024 19:15
@alenakhineika alenakhineika merged commit 20caf07 into main Jun 26, 2024
5 checks passed
@alenakhineika alenakhineika deleted the bump-minor-deps branch June 26, 2024 22:33
@mabaasit mabaasit mentioned this pull request Jun 27, 2024
11 tasks
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.

2 participants