-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Remove unusedDocuments
, fix culling of foreign documents
#15105
Conversation
Thanks for making a pull request to jupyterlab! |
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.
Thanks @krassowski
document.remainingLifetime -= 1; | ||
if (document.remainingLifetime <= 0) { | ||
document.dispose(); | ||
const ids = documentIDs.get(document)!; | ||
for (const id of ids) { | ||
this.foreignDocuments.delete(id); |
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.
Should we add a test for this case?
These were needed in LSP because it subclasses `VirtualDocument`, but are not needed upstream in JupyterLab itself.
@meeseeksdev please backport to 4.0.x |
…f foreign documents
…documents (#15121) Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
References
Follow up to #12534
Developed in response to a failing test caught on jupyter-lsp/jupyterlab-lsp#978
Code changes
unusedDocuments
is removed (it was never written to since Integratejupyterlab-lsp
intojupyterlab
#12534, it was functioning properly with different logic in lsp extension)closeExpiredDocuments()
without need to keep state on theVirtualDocument
instanceUser-facing changes
When user creates a markdown or raw cell and then deletes it (without other cells of this type present), the connection to LSP server is not held indefinitely but culled after six further document changes.
Backwards-incompatible changes
Removal of the protected
unusedDocuments
method is notable but we are not advertising LSP as stable yet, and it would be useless in the current state anyways so it should be fine.