sendOpen after connection handlers are installed #202
Conversation
|
Some notes:
It looks like when diagnostics are not showing, perhaps the document hasn't been fully initialized. |
|
Looks like the event is being fired before the diagnostic listener is actually installed: So apparently, the docker server is just really fast to initialize and report diagnostics. My first attempt will be to have adapter take over signaling that it wants to open a document by enqueuing this with the connection, rather than have the connection manager handle it... |
| @@ -294,21 +294,29 @@ export abstract class JupyterLabWidgetAdapter | |||
| this.on_foreign_document_opened, | |||
| this | |||
| ); | |||
|
|
|||
| await this.connect(virtual_document).catch(console.warn); | |||
| return await this.connect(virtual_document).catch(console.warn); | |||
There was a problem hiding this comment.
rather than changing the output signature of this, we could change the input signature and add an deferOpen = false: foreign documents would be opened immediately, presumably, as all the features would already be connected.
There was a problem hiding this comment.
yes, please - this would make it much easier to understand in the future!
There was a problem hiding this comment.
sure, pushed this around. on_foreign and reload_connection will both try to sendOpen as the adapter will have been fully initialized by that point, i'm pretty sure.
| this.documentsToOpen = []; | ||
| } | ||
|
|
||
| sendOpenWhenReady(documentInfo: IDocumentInfo) { |
There was a problem hiding this comment.
not sure if this is the best way, but seemed the most straightforward. i guess it could notify that it did open each document, but nobody would be listening right now.
There was a problem hiding this comment.
Yes, we are not allowed to send anything before receiving InitializeResult:
Until the server has responded to the initialize request with an InitializeResult, the client must not send any additional requests or notifications to the server
There was a problem hiding this comment.
we should change ws-connection to complain much more loudly about that... but maybe not on this PR.
There was a problem hiding this comment.
another approach: once this is in, instead of failing (or dropping messages on the floor, as it currently does), add Connection.ready: Promise<void>, and have everything except initialize do an await on that. Would have to do some digging around to see if that's viable.
There was a problem hiding this comment.
tried this out on a branch... breaks a bunch of things. doesn't mean it's not worth doing, just needs more than some simple checks...
bollwyvl
changed the title
|
Checking it out on binder (link added to top), but pretty bullish on this fixing #201. |
|
Yep, looks good on binder. |
| @@ -31,6 +31,9 @@ jobs: | |||
| - script: ${{ parameters.env_docs }} || ${{ parameters.env_docs }} || ${{ parameters.env_docs }} | |||
| displayName: docs dependencies | |||
|
|
|||
| - script: ${{ platform.activate }} && conda uninstall -y pytest-cov | |||
| displayName: remove coverage to prevent warning | |||
There was a problem hiding this comment.
adding this to remove warning, but also to trigger another run (haven't found anything else worth trying/changing)
There was a problem hiding this comment.
hopefully right/faster now...
There was a problem hiding this comment.
now takes 6s rather than 1min, though I did forget to add pip to the env... we'll see how the current run goes: not strictly required for merge.
|
Thanks for the quick reviews. Provided this passes (doing a full run locally, too), and you don't see any other glaring issues, I don't plan on doing anything else on it at this time! 🚀 🎸 |
|
Local test looks good, trying binder. |
|
last fail was pure-and-simple azure flake.. |
|
Back to happy... |
|
Thank you. |
References
Code changes
"dockerfile-language-server-nodejs": "^0.0.22",User-facing changes
Backwards-incompatible changes
N/A
Chores