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

sendOpen after connection handlers are installed #202

Merged
merged 9 commits into from
Feb 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
- ignores malformed diagnostic ranges, enabling markdown support ([#165][])
- passes tests on Python 3.8 on Windows ([#165][])

- bug fixes

- reports files are open only after installing all handlers to avoid missing messages ([#201][])
krassowski marked this conversation as resolved.
Show resolved Hide resolved

[#201]: https://github.com/krassowski/jupyterlab-lsp/issues/201

### `lsp-ws-connection 0.4.0` (unreleased)

- breaking changes
Expand Down
3 changes: 3 additions & 0 deletions ci/job.docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ jobs:
- script: ${{ parameters.env_docs }} || ${{ parameters.env_docs }} || ${{ parameters.env_docs }}
displayName: docs dependencies

- script: ${{ platform.activate }} jupyterlab-lsp && conda uninstall -y --force-remove pytest-cov
displayName: remove coverage to prevent warning
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding this to remove warning, but also to trigger another run (haven't found anything else worth trying/changing)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hopefully right/faster now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


- script: ${{ platform.activate }} jupyterlab-lsp && jlpm || jlpm || jlpm
displayName: install npm dependencies (for language servers)

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
},
"devDependencies": {
"bash-language-server": "^1.6.1",
"dockerfile-language-server-nodejs": "^0.0.21",
"dockerfile-language-server-nodejs": "^0.0.22",
krassowski marked this conversation as resolved.
Show resolved Hide resolved
"eslint": "^5.16.0",
"eslint-config-prettier": "^4.1.0",
"eslint-plugin-prettier": "^3.0.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ export class FileEditorAdapter extends JupyterLabWidgetAdapter {
this.connect_contentChanged_signal();

console.log('LSP: file ready for connection:', this.path);
this.connect_document(this.virtual_editor.virtual_document).catch(

// connect the document, but do not open it as the adapter will handle this
// after registering all features
this.connect_document(this.virtual_editor.virtual_document, false).catch(
console.warn
);

Expand Down
44 changes: 40 additions & 4 deletions packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ export abstract class JupyterLabWidgetAdapter
// recreate virtual document using current path and language
this.virtual_editor.create_virtual_document();
// reconnect
this.connect_document(this.virtual_editor.virtual_document).catch(
this.connect_document(this.virtual_editor.virtual_document, true).catch(
console.warn
);
}
Expand Down Expand Up @@ -287,23 +287,57 @@ export abstract class JupyterLabWidgetAdapter
});
}

protected async connect_document(virtual_document: VirtualDocument) {
/**
* Opens a connection for the document. The connection may or may
* not be initialized, yet, and depending on when this is called, the client
* may not be fully connected.
*
* @param virtual_document a VirtualDocument
* @param send_open whether to open the document immediately
*/
protected async connect_document(
virtual_document: VirtualDocument,
send_open = false
): Promise<void> {
virtual_document.changed.connect(this.document_changed, this);

virtual_document.foreign_document_opened.connect(
this.on_foreign_document_opened,
this
);

await this.connect(virtual_document).catch(console.warn);
const connection_context = await this.connect(virtual_document).catch(
console.warn
);

if (!send_open) {
return;
}

if (connection_context && connection_context.connection) {
connection_context.connection.sendOpenWhenReady(
virtual_document.document_info
);
} else {
console.warn(`Connection for ${virtual_document.path} was not opened`);
}
}

/**
* Handler for opening a document contained in a parent document. The assumption
* is that the editor already exists for this, and as such the document
* should be queued for immediate opening.
*
* @param host the VirtualDocument that contains the VirtualDocument in another language
* @param context information about the foreign VirtualDocument
*/
protected async on_foreign_document_opened(
host: VirtualDocument,
context: IForeignContext
) {
const { foreign_document } = context;
this.connect_document(foreign_document).catch(console.warn);

await this.connect_document(foreign_document, true);

foreign_document.foreign_document_closed.connect(
this.on_foreign_document_closed,
Expand Down Expand Up @@ -453,6 +487,8 @@ export abstract class JupyterLabWidgetAdapter
adapter_features
);
console.log('LSP: Adapter for', this.document_path, 'is ready.');
// the client is now fully ready: signal to the server that the document is "open"
connection.sendOpenWhenReady(virtual_document.document_info);
return adapter;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/jupyterlab-lsp/src/adapters/jupyterlab/notebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ export class NotebookAdapter extends JupyterLabWidgetAdapter {
);
this.connect_contentChanged_signal();

this.connect_document(this.virtual_editor.virtual_document).catch(
// connect the document, but do not open it as the adapter will handle this
// after registering all features
this.connect_document(this.virtual_editor.virtual_document, false).catch(
console.warn
);
}
Expand Down
18 changes: 18 additions & 0 deletions packages/jupyterlab-lsp/src/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,26 @@ import { until_ready } from './utils';
interface ILSPOptions extends ILspOptions {}

export class LSPConnection extends LspWsConnection {
protected documentsToOpen: IDocumentInfo[];

constructor(options: ILSPOptions) {
super(options);
this.documentsToOpen = [];
}

sendOpenWhenReady(documentInfo: IDocumentInfo) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should change ws-connection to complain much more loudly about that... but maybe not on this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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...

if (this.isReady) {
this.sendOpen(documentInfo);
} else {
this.documentsToOpen.push(documentInfo);
}
}

protected onServerInitialized(params: lsProtocol.InitializeResult) {
super.onServerInitialized(params);
while (this.documentsToOpen.length) {
this.sendOpen(this.documentsToOpen.pop());
}
}

public sendSelectiveChange(
Expand Down
5 changes: 0 additions & 5 deletions packages/jupyterlab-lsp/src/connection_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,6 @@ export class DocumentConnectionManager {
// be re-opened and synced
this.connections.set(virtual_document.id_path, connection);

if (connection.isReady) {
connection.sendOpen(virtual_document.document_info);
}

return connection;
}

Expand Down Expand Up @@ -183,7 +179,6 @@ export class DocumentConnectionManager {

connection.on('serverInitialized', capabilities => {
this.forEachDocumentOfConnection(connection, virtual_document => {
connection.sendOpen(virtual_document.document_info);
// TODO: is this still neccessary, e.g. for status bar to update responsively?
this.initialized.emit({ connection, virtual_document });
});
Expand Down
1 change: 1 addition & 0 deletions requirements/docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ channels:
dependencies:
- nbsphinx
- pandas
- pip
- pytest
- python-graphviz
- recommonmark
Expand Down
76 changes: 51 additions & 25 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,7 @@
typestyle "^2.0.1"

"@krassowski/jupyterlab-lsp@file:packages/jupyterlab-lsp":
version "0.7.0"
version "0.7.1"
dependencies:
"@krassowski/jupyterlab_go_to_definition" "^0.7.1"
lsp-ws-connection "0.3.1"
Expand Down Expand Up @@ -4315,30 +4315,31 @@ [email protected]:
dependencies:
vscode-languageserver-types "^3.5.0"

[email protected].16:
version "0.0.16"
resolved "https://registry.yarnpkg.com/dockerfile-ast/-/dockerfile-ast-0.0.16.tgz#10b329d343329dab1de70375833495f85ad65913"
integrity sha512-+HZToHjjiLPl46TqBrok5dMrg5oCkZFPSROMQjRmvin0zG4FxK0DJXTpV/CUPYY2zpmEvVza55XLwSHFx/xZMw==
[email protected].20:
version "0.0.20"
resolved "https://registry.yarnpkg.com/dockerfile-ast/-/dockerfile-ast-0.0.20.tgz#fd1ce404b5a8b414a9a4d600bcc9ca64a42fd659"
integrity sha512-VGFMBT0Av1Sk4SCjafsv2S/MCVy6+AuhLW+958wS2df86wb90JPl+WJXhTTGHk5DQVwQX0NoQQBzQQP1U7GaWg==
dependencies:
vscode-languageserver-types "^3.5.0"

dockerfile-language-server-nodejs@^0.0.21:
version "0.0.21"
resolved "https://registry.yarnpkg.com/dockerfile-language-server-nodejs/-/dockerfile-language-server-nodejs-0.0.21.tgz#cb6a526524d5ea5a2580b56cd3ce3102ba6384fc"
integrity sha512-lZ7VFAlS4vTm5MvxmwpREcYMARB3RQaGX0OZdcY8oSytsu4i5mMGVa6mi9/pZ9soqcUC08uxEA8EcqIeL3lyAA==
dockerfile-language-server-nodejs@^0.0.22:
version "0.0.22"
resolved "https://registry.yarnpkg.com/dockerfile-language-server-nodejs/-/dockerfile-language-server-nodejs-0.0.22.tgz#9b214988b16b7c32986a66741e1f6eb88da62385"
integrity sha512-Vf/Zieb/BBs/VQnaxntshlTExR3FyE6FO1NxS+yO3SVqzcEVHYkHMC8f/+XRRROVHFh41YfzVfPhSxdCxfbviQ==
dependencies:
dockerfile-language-service "0.0.8"
dockerfile-language-service "0.0.9"
dockerfile-utils "0.0.11"
vscode-languageserver "^5.1.0"
vscode-languageserver "^6.1.0"

[email protected].8:
version "0.0.8"
resolved "https://registry.yarnpkg.com/dockerfile-language-service/-/dockerfile-language-service-0.0.8.tgz#12c1e7fa5dc8152af915e1e52bd0303d4c540231"
integrity sha512-peko1rQtZ81e3QK3VLZgkCKkCMnuoSqZxCQuudyEbtYqqAe6jJ4r0bwOnWD9hyH/FMg7jMvI5uLsoo7/dHLNYw==
[email protected].9:
version "0.0.9"
resolved "https://registry.yarnpkg.com/dockerfile-language-service/-/dockerfile-language-service-0.0.9.tgz#75e38a523b490732bac538773447c15c01ecddf4"
integrity sha512-g+TFMRG/Vv+yKqYJ2EE5KZlmwbPShWhlGhyG6tFEhUlHUt2Cd3wMr35popmc5Y9ra3OPwR3nY9cQFWIt8OP1Kw==
dependencies:
dockerfile-ast "0.0.16"
dockerfile-utils "0.0.13"
vscode-languageserver-types "^3.10.0"
dockerfile-ast "0.0.20"
dockerfile-utils "0.0.14"
vscode-languageserver-protocol "^3.15.1"
vscode-languageserver-types "^3.15.1"

[email protected]:
version "0.0.11"
Expand All @@ -4348,12 +4349,12 @@ [email protected]:
dockerfile-ast "0.0.12"
vscode-languageserver-types "3.6.0"

[email protected].13:
version "0.0.13"
resolved "https://registry.yarnpkg.com/dockerfile-utils/-/dockerfile-utils-0.0.13.tgz#43dd955e6c4d2c4c844216b7128d5a95b8fdc783"
integrity sha512-+MAmhEnQ16B7+3C2UDWpmIS1D8EiKvpl4LDRjrMv94bOusaeRcLagRR0AvgV6NWT+oiRxDMLDyay6yjm6LESsw==
[email protected].14:
version "0.0.14"
resolved "https://registry.yarnpkg.com/dockerfile-utils/-/dockerfile-utils-0.0.14.tgz#76e053d430cfa08b7a9480c03a4c8f8816877177"
integrity sha512-9S77f18SmnI4hJ1Ndv1h1/gPxm74uV/n9E/2JMp6I9D3cSLrNdBZwq3FpNiXX1TRJQAn+Ufl/5b7YzH63NZ6jA==
dependencies:
dockerfile-ast "0.0.16"
dockerfile-ast "0.0.20"
vscode-languageserver-types "3.6.0"

doctrine@^3.0.0:
Expand Down Expand Up @@ -11819,6 +11820,11 @@ vscode-jsonrpc@^4.1.0-next:
resolved "https://registry.yarnpkg.com/vscode-jsonrpc/-/vscode-jsonrpc-4.1.0-next.3.tgz#05fe742959a2726020d4d0bfbc3d3c97873c7fde"
integrity sha512-Z6oxBiMks2+UADV1QHXVooSakjyhI+eHTnXzDyVvVMmegvSfkXk2w6mPEdSkaNHFBdtWW7n20H1yw2nA3A17mg==

vscode-jsonrpc@^5.0.1:
version "5.0.1"
resolved "https://registry.yarnpkg.com/vscode-jsonrpc/-/vscode-jsonrpc-5.0.1.tgz#9bab9c330d89f43fc8c1e8702b5c36e058a01794"
integrity sha512-JvONPptw3GAQGXlVV2utDcHx0BiY34FupW/kI6mZ5x06ER5DdPG/tXWMVHjTNULF5uKPOUUD0SaXg5QaubJL0A==

vscode-languageclient@^5.2.1:
version "5.2.1"
resolved "https://registry.yarnpkg.com/vscode-languageclient/-/vscode-languageclient-5.2.1.tgz#7cfc83a294c409f58cfa2b910a8cfeaad0397193"
Expand Down Expand Up @@ -11851,16 +11857,29 @@ [email protected]:
vscode-jsonrpc "3.5.0"
vscode-languageserver-types "3.5.0"

vscode-languageserver-protocol@^3.15.1, vscode-languageserver-protocol@^3.15.3:
version "3.15.3"
resolved "https://registry.yarnpkg.com/vscode-languageserver-protocol/-/vscode-languageserver-protocol-3.15.3.tgz#3fa9a0702d742cf7883cb6182a6212fcd0a1d8bb"
integrity sha512-zrMuwHOAQRhjDSnflWdJG+O2ztMWss8GqUUB8dXLR/FPenwkiBNkMIJJYfSN6sgskvsF0rHAoBowNQfbyZnnvw==
dependencies:
vscode-jsonrpc "^5.0.1"
vscode-languageserver-types "3.15.1"

vscode-languageserver-textdocument@^1.0.0-next.4:
version "1.0.0-next.5"
resolved "https://registry.yarnpkg.com/vscode-languageserver-textdocument/-/vscode-languageserver-textdocument-1.0.0-next.5.tgz#dbb7a45dd973a19261a7c57ab9a439c40f3799ee"
integrity sha512-1jp/zAidN/bF/sqPimhBX1orH5G4rzRw63k75TesukJDuxm8yW79ECStWbDSy41BHGOwSGN4M69QFvhancSr5A==

[email protected], vscode-languageserver-types@^3.0.3, vscode-languageserver-types@^3.10.0, vscode-languageserver-types@^3.13.0, vscode-languageserver-types@^3.14.0, vscode-languageserver-types@^3.5.0, vscode-languageserver-types@^3.6.1, vscode-languageserver-types@^3.7.2:
[email protected], vscode-languageserver-types@^3.0.3, vscode-languageserver-types@^3.13.0, vscode-languageserver-types@^3.14.0, vscode-languageserver-types@^3.6.1, vscode-languageserver-types@^3.7.2:
version "3.14.0"
resolved "https://registry.yarnpkg.com/vscode-languageserver-types/-/vscode-languageserver-types-3.14.0.tgz#d3b5952246d30e5241592b6dde8280e03942e743"
integrity sha512-lTmS6AlAlMHOvPQemVwo3CezxBp0sNB95KNPkqp3Nxd5VFEnuG1ByM0zlRWos0zjO3ZWtkvhal0COgiV1xIA4A==

[email protected], vscode-languageserver-types@^3.15.1, vscode-languageserver-types@^3.5.0:
version "3.15.1"
resolved "https://registry.yarnpkg.com/vscode-languageserver-types/-/vscode-languageserver-types-3.15.1.tgz#17be71d78d2f6236d414f0001ce1ef4d23e6b6de"
integrity sha512-+a9MPUQrNGRrGU630OGbYVQ+11iOIovjCkqxajPa9w57Sd5ruK8WQNsslzpa0x/QJqC8kRc2DUxWjIFwoNm4ZQ==

[email protected]:
version "3.5.0"
resolved "https://registry.yarnpkg.com/vscode-languageserver-types/-/vscode-languageserver-types-3.5.0.tgz#e48d79962f0b8e02de955e3f524908e2b19c0374"
Expand Down Expand Up @@ -11892,14 +11911,21 @@ vscode-languageserver@^4.0.0, vscode-languageserver@^4.1.3:
vscode-languageserver-protocol "^3.10.3"
vscode-uri "^1.0.5"

vscode-languageserver@^5.0.0, vscode-languageserver@^5.1.0, vscode-languageserver@^5.2.1:
vscode-languageserver@^5.0.0, vscode-languageserver@^5.2.1:
version "5.2.1"
resolved "https://registry.yarnpkg.com/vscode-languageserver/-/vscode-languageserver-5.2.1.tgz#0d2feddd33f92aadf5da32450df498d52f6f14eb"
integrity sha512-GuayqdKZqAwwaCUjDvMTAVRPJOp/SLON3mJ07eGsx/Iq9HjRymhKWztX41rISqDKhHVVyFM+IywICyZDla6U3A==
dependencies:
vscode-languageserver-protocol "3.14.1"
vscode-uri "^1.0.6"

vscode-languageserver@^6.1.0:
version "6.1.1"
resolved "https://registry.yarnpkg.com/vscode-languageserver/-/vscode-languageserver-6.1.1.tgz#d76afc68172c27d4327ee74332b468fbc740d762"
integrity sha512-DueEpkUAkD5XTR4MLYNr6bQIp/UFR0/IPApgXU3YfCBCB08u2sm9hRCs6DxYZELkk++STPjpcjksR2H8qI3cDQ==
dependencies:
vscode-languageserver-protocol "^3.15.3"

vscode-nls@^2.0.2:
version "2.0.2"
resolved "https://registry.yarnpkg.com/vscode-nls/-/vscode-nls-2.0.2.tgz#808522380844b8ad153499af5c3b03921aea02da"
Expand Down