-
Notifications
You must be signed in to change notification settings - Fork 507
Refactor: Define Webview RPC Contracts Using JSON-RPC syntax to added typings #19613
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
Conversation
…import handling and error management
…quest and notification types for improved communication and state management
…n structure - Updated VscodeWebviewProvider to utilize sendRequest and sendNotification for theme, state, and localization retrieval. - Refactored ConnectionDialogStateProvider to replace call with sendRequest for getting connection display names. - Modified CommandBar and QueryResultPane to use sendRequest for saving results and loading rows. - Enhanced table plugins to adopt sendRequest for clipboard operations and filter management. - Updated SchemaDesignerStateProvider to utilize sendRequest for initializing and managing schema sessions. - Introduced new request types in sharedInterfaces for better type safety and clarity in RPC communication.
…otification structure for improved communication and state management
src/reactviews/common/rpc.ts
Outdated
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.
Please review this file more carefully as it actually defines the new architecture.
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.
Please review this file more carefully as it actually defines the new architecture.
… banned imports in tests
…ity and maintainability
…fication types for improved clarity and maintainability
PR Changes
|
@@ -4,7 +4,7 @@ | |||
*--------------------------------------------------------------------------------------------*/ | |||
|
|||
import { ExpandParams } from "./expandNodeRequest"; | |||
import { RequestType } from "vscode-jsonrpc"; | |||
import { RequestType } from "vscode-languageclient"; |
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.
Why this change?
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.
Keeping it consistent with other mssql contracts.
…c to improve clarity and consistency in request resolution
…roved accessibility in ReactWebviewBaseController
…e arrow function syntax for improved consistency and readability
…n registerCommonRequestHandlers
…rs in registerCommonRequestHandlers
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.
LGTM overall, I did extensive testing in query result area and there's no issue found other the ones I mentioned earlier.
Pull Request Template – vscode-mssql
Description
This PR updates our webview communication to follow a more structured JSON-RPC style by introducing strongly typed contracts.
We now support bidirectional communication between webviews and their controllers via:
onRequest
sendRequest
onNotification
sendNotification
This change brings our webview architecture closer to the existing STS ↔ extension communication model. It unifies the syntax, improves type safety, and makes development and debugging much easier.
One of the biggest benefits is improved discoverability. You can now easily trace who is calling or handling a request just by checking the references of the request type:
In this screenshot,
GetPlatformRequest
clearly shows both the caller and the handler, making things more maintainable.I'll explore a similar pattern for refactoring reducers in a future PR.
Code Changes Checklist
npm run test
)Reviewers: Please read our reviewer guidelines