Show an error notification when misconfigured #79
Conversation
| webSocket.addEventListener('open', () => resolve(conn)) | ||
| webSocket.addEventListener('error', event => reject(new Error(connectingToGoLangserverHelp))) | ||
| webSocket.addEventListener('error', event => { | ||
| notifyUnableToConnectToLanguageServer(address) |
There was a problem hiding this comment.
This seems redundant - this rejects the Promise, which is caught in the catch Block, where this function is also called. In general it's not good to have this logic in the event handler because if notifyUnableToConnectToLanguageServer throws itself for some reason that error will not be propagated and the Promise will be pending forever.
| reject(event) | ||
| }) | ||
| } catch (e) { | ||
| if ('message' in e && /Failed to construct/.test(e.message)) { |
There was a problem hiding this comment.
This err.message check (as any err.message check really) seems really brittle. What case is it trying to detect? Which case is it trying to not detect?
For example, I notice that it gets triggered in Chrome when you pass an http:// URL. Firefox however has a different Error message for that case. It also does not get triggered for an unsuccessful connection, or for passing an invalid URL.
| ].join('\n') | ||
|
|
||
| const connection = (await new Promise((resolve, reject) => { | ||
| const connection = (await new Promise<wsrpc.MessageConnection>((resolve, reject) => { |
There was a problem hiding this comment.
This should use an async IIFE instead of the Promise constructor (Promise constructor should be avoided where possible, and it's possible here)
| console.error(connectingToSourcegraphHelp) | ||
| if ( | ||
| 'message' in e && | ||
| (/no such host/.test(e.message) || /i\/o timeout/.test(e.message) || /connection refused/.test(e.message)) |
There was a problem hiding this comment.
Where do these errors come from? How can we be confident that these error messages are reliable? Are there no error codes/types, and if not, can we add them?
Which errors can happen that should not pass through this?
| 'codeIntel.error.link': 'https://sourcegraph.com/extensions/sourcegraph/go', | ||
| }) | ||
| const portByProtocol: { [protocol: string]: string } = { 'wss:': '443', 'ws:': '80' } | ||
| sourcegraph.app.activeWindow!.showNotification( |
There was a problem hiding this comment.
If it's an error message, it should use NotificationType.Error
| let webSocket: WebSocket | ||
| try { | ||
| webSocket = new WebSocket(address.href) | ||
| } catch (e) { |
There was a problem hiding this comment.
This error would get caught by the outer catch, which has the same handling, so this try/catch appears redundant
| notifyUnableToConnectToLanguageServer(address) | ||
| return | ||
| } | ||
| webSocket.addEventListener('open', () => resolve()) |
There was a problem hiding this comment.
| webSocket.addEventListener('open', () => resolve()) | |
| webSocket.addEventListener('open', resolve) |
| return | ||
| } | ||
| webSocket.addEventListener('open', () => resolve()) | ||
| webSocket.addEventListener('error', error => reject(error)) |
There was a problem hiding this comment.
error is not an Error, it's an Event - always reject with Error instances
| ) | ||
| } | ||
|
|
||
| async function canary(address: URL): Promise<void> { |
There was a problem hiding this comment.
Could you add a doc comment? It's not obvious to me what this function's job is (maybe I just don't understand the name)
| `- Comment out **\`go.serverUrl\`** in your [global settings](${ | ||
| sourcegraph.internal.sourcegraphURL | ||
| }site-admin/global-settings).`, | ||
| ].join('\n') |
|
This looks like yet another thing that we should have for all language extensions/servers, not just Go. It should be added to |
|
Thanks for the useful feedback 🙇 If we can get lsp-client integrated soon, I'll migrate this PR to lsp-client and then simply bump the version in sourcegraph-go. |
When your
go.serverUrlis wrong, you'll see:When your
go.sourcegraphUrlis wrong, you'll see:@ryan-blunden Any feedback on this second error message?
The icon links to https://sourcegraph.com/extensions/sourcegraph/go
Only admins see these error notifications.
cc @ryan-blunden who wordsmithed the first error message