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

server: Only perform dynamic registration if client supports it #763

Merged

Conversation

the-mikedavis
Copy link
Contributor

The server shouldn't send a client/registerCapability unless the client signals support with the dynamicRegistration client capability set to true for didChangeConfiguration. This change checks that field before attempting to register for config change notifications.

This change is important for clients that don't support dynamic capability registration: if we reply to the registerCapability request with a MethodNotHandled JSONRPC error, the promise from connection.client.register is rejected which causes the server to exit.

The server shouldn't send a `client/registerCapability` unless the
client signals support with the dynamicRegistration client capability
set to true for didChangeConfiguration. This change checks that field
before attempting to register for config change notifications.
Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for fixing this.

Are you comfortable bumping the server version inside server/package.json with a minor increase? It would also be great if you can add an entry in the server/CHANGELOG file.

@the-mikedavis
Copy link
Contributor Author

Sure thing! I bumped the patch version since I see this as a small fix but I can bump this to 4.9.0 instead if you prefer

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

👏

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