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(amazonq): acquire Toolkit api on-demand #4874
base: master
Are you sure you want to change the base?
Conversation
e9a81f4
to
60b1f40
Compare
label: conn.label, | ||
} as AwsConnection) | ||
} | ||
const toolkitApi = getToolkitApi() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get the API on-demand. Simpler, fewer edge cases, reliable, easier to reason about.
return undefined | ||
} | ||
await activateExtension(VSCODE_EXTENSION_ID.awstoolkit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add this back, but it's kind of pointless because Toolkit activates on onStartupFinished
.
Problem: The logic to get the Toolkit API is spread around and involves: - a "aws.amazonq.refreshConnectionCallback" command (silent failures, not strongly typed, etc) - numerous attempts to "eagerly" activate Toolkit - Toolkit's own startup passes the toolkit API as an object via the "aws.amazonq.refreshConnectionCallback" command - this might not work if the object doesn't serialize correctly, e.g. for web workers or when the vscode "extensions.experimental.affinity" behavior is enabled. Solution: Instead of trying to eagerly activate Toolkit, just get the Toolkit API on-demand. This avoids complexity and is more reliable.
60b1f40
to
1f77a2a
Compare
Can you also update how toolkit api is acquired in |
@@ -430,27 +459,6 @@ const registerToolkitApiCallbackOnce = once(async () => { | |||
} | |||
) | |||
} | |||
}) | |||
export const registerToolkitApiCallback = Commands.declare( | |||
{ id: 'aws.amazonq.refreshConnectionCallback' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this command is removed, this change has to go through both toolkit and Q. Assume it is released in 1.0.1 Q and 3.0.1 Toolkit, then there can be command not found issue when using 1.0.1 Q with 3.0.0 toolkit or using 1.0.0 Q with 3.0.1 toolkit. We do wrap it under tryExecute so user should not see a command not found popup. This is not a blocker issue since user can upgrade both (but both needs to go out at same time)
(This can wait until after release)
Problem:
The logic to get the Toolkit API is spread around and involves:
Solution:
Instead of trying to eagerly activate Toolkit, just get the Toolkit API on-demand. This avoids complexity and is more reliable.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.