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

Use content providers to remove RTC prefix #418

Merged
merged 14 commits into from
Jan 7, 2025

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Dec 16, 2024

We still need to decide what to do where RTC: prefix was used in the actual logic.

One of my goals here is to keep the diff minimal to make the review easier.

Co-authored-by: davidbrochart <[email protected]>
@krassowski krassowski added the enhancement New feature or request label Dec 16, 2024
Copy link
Contributor

Binder 👈 Launch a Binder on branch krassowski/jupyter_collaboration/content-provider-v2

@krassowski krassowski marked this pull request as ready for review December 17, 2024 11:50
@krassowski
Copy link
Member Author

CI is expected to fail until jupyterlab/jupyterlab#17092 is merged and released.

@davidbrochart
Copy link
Collaborator

I installed jupyterlab/jupyterlab#17092 and then this PR, but I have an error:

> @jupyter/collaborative-drive:"install:extension"
src/tokens.ts(5,20): error TS2305: Module '"@jupyterlab/services"' has no exported member 'SharedDocumentFactory'.

Am I missing something?

@krassowski
Copy link
Member Author

SharedDocumentFactory is definitely exported in jupyterlab/jupyterlab#17092.

Did you link the packages from jupyterlab/jupyterlab#17092? I used:

jlpm link /path-to-jupyterlab-packages/ --all
jlpm
jlpm dlx yarn-berry-deduplicate

I had to add "skipLibCheck": true, to silence some react version warnings and added some resolutions to pin versions, see below:

diff --git a/package.json b/package.json
index 8520d4e..6860e66 100644
--- a/package.json
+++ b/package.json
@@ -66,5 +66,116 @@
     "stylelint-prettier": "^3.0.0",
     "typedoc": "~0.23.28",
     "typescript": "~5.0.4"
+  },
+  "resolutions": {
+    "@jupyterlab/services": "portal:/home/krassowski/jupyterlab/packages/services",
+    "@jupyterlab/docmanager": "portal:/home/krassowski/jupyterlab/packages/docmanager",
+    "@jupyterlab/builder": "portal:/home/krassowski/jupyterlab/builder",
+    "@jupyterlab/buildutils": "portal:/home/krassowski/jupyterlab/buildutils",
+    "@jupyterlab/template": "portal:/home/krassowski/jupyterlab/buildutils/template",
+    "@jupyterlab/galata": "portal:/home/krassowski/jupyterlab/galata",
+    "@jupyterlab/application": "portal:/home/krassowski/jupyterlab/packages/application",
+    "@jupyterlab/application-extension": "portal:/home/krassowski/jupyterlab/packages/application-extension",
+    "@jupyterlab/apputils": "portal:/home/krassowski/jupyterlab/packages/apputils",
+    "@jupyterlab/apputils-extension": "portal:/home/krassowski/jupyterlab/packages/apputils-extension",
+    "@jupyterlab/attachments": "portal:/home/krassowski/jupyterlab/packages/attachments",
+    "@jupyterlab/cell-toolbar": "portal:/home/krassowski/jupyterlab/packages/cell-toolbar",
+    "@jupyterlab/cell-toolbar-extension": "portal:/home/krassowski/jupyterlab/packages/cell-toolbar-extension",
+    "@jupyterlab/cells": "portal:/home/krassowski/jupyterlab/packages/cells",
+    "@jupyterlab/celltags-extension": "portal:/home/krassowski/jupyterlab/packages/celltags-extension",
+    "@jupyterlab/codeeditor": "portal:/home/krassowski/jupyterlab/packages/codeeditor",
+    "@jupyterlab/codemirror": "portal:/home/krassowski/jupyterlab/packages/codemirror",
+    "@jupyterlab/codemirror-extension": "portal:/home/krassowski/jupyterlab/packages/codemirror-extension",
+    "@jupyterlab/completer": "portal:/home/krassowski/jupyterlab/packages/completer",
+    "@jupyterlab/completer-extension": "portal:/home/krassowski/jupyterlab/packages/completer-extension",
+    "@jupyterlab/console": "portal:/home/krassowski/jupyterlab/packages/console",
+    "@jupyterlab/console-extension": "portal:/home/krassowski/jupyterlab/packages/console-extension",
+    "@jupyterlab/coreutils": "portal:/home/krassowski/jupyterlab/packages/coreutils",
+    "@jupyterlab/csvviewer": "portal:/home/krassowski/jupyterlab/packages/csvviewer",
+    "@jupyterlab/csvviewer-extension": "portal:/home/krassowski/jupyterlab/packages/csvviewer-extension",
+    "@jupyterlab/debugger": "portal:/home/krassowski/jupyterlab/packages/debugger",
+    "@jupyterlab/debugger-extension": "portal:/home/krassowski/jupyterlab/packages/debugger-extension",
+    "@jupyterlab/docmanager-extension": "portal:/home/krassowski/jupyterlab/packages/docmanager-extension",
+    "@jupyterlab/docregistry": "portal:/home/krassowski/jupyterlab/packages/docregistry",
+    "@jupyterlab/documentsearch": "portal:/home/krassowski/jupyterlab/packages/documentsearch",
+    "@jupyterlab/documentsearch-extension": "portal:/home/krassowski/jupyterlab/packages/documentsearch-extension",
+    "@jupyterlab/extensionmanager": "portal:/home/krassowski/jupyterlab/packages/extensionmanager",
+    "@jupyterlab/extensionmanager-extension": "portal:/home/krassowski/jupyterlab/packages/extensionmanager-extension",
+    "@jupyterlab/filebrowser": "portal:/home/krassowski/jupyterlab/packages/filebrowser",
+    "@jupyterlab/filebrowser-extension": "portal:/home/krassowski/jupyterlab/packages/filebrowser-extension",
+    "@jupyterlab/fileeditor": "portal:/home/krassowski/jupyterlab/packages/fileeditor",
+    "@jupyterlab/fileeditor-extension": "portal:/home/krassowski/jupyterlab/packages/fileeditor-extension",
+    "@jupyterlab/help-extension": "portal:/home/krassowski/jupyterlab/packages/help-extension",
+    "@jupyterlab/htmlviewer": "portal:/home/krassowski/jupyterlab/packages/htmlviewer",
+    "@jupyterlab/htmlviewer-extension": "portal:/home/krassowski/jupyterlab/packages/htmlviewer-extension",
+    "@jupyterlab/hub-extension": "portal:/home/krassowski/jupyterlab/packages/hub-extension",
+    "@jupyterlab/imageviewer": "portal:/home/krassowski/jupyterlab/packages/imageviewer",
+    "@jupyterlab/imageviewer-extension": "portal:/home/krassowski/jupyterlab/packages/imageviewer-extension",
+    "@jupyterlab/inspector": "portal:/home/krassowski/jupyterlab/packages/inspector",
+    "@jupyterlab/inspector-extension": "portal:/home/krassowski/jupyterlab/packages/inspector-extension",
+    "@jupyterlab/javascript-extension": "portal:/home/krassowski/jupyterlab/packages/javascript-extension",
+    "@jupyterlab/json-extension": "portal:/home/krassowski/jupyterlab/packages/json-extension",
+    "@jupyterlab/launcher": "portal:/home/krassowski/jupyterlab/packages/launcher",
+    "@jupyterlab/launcher-extension": "portal:/home/krassowski/jupyterlab/packages/launcher-extension",
+    "@jupyterlab/logconsole": "portal:/home/krassowski/jupyterlab/packages/logconsole",
+    "@jupyterlab/logconsole-extension": "portal:/home/krassowski/jupyterlab/packages/logconsole-extension",
+    "@jupyterlab/lsp": "portal:/home/krassowski/jupyterlab/packages/lsp",
+    "@jupyterlab/lsp-extension": "portal:/home/krassowski/jupyterlab/packages/lsp-extension",
+    "@jupyterlab/mainmenu": "portal:/home/krassowski/jupyterlab/packages/mainmenu",
+    "@jupyterlab/mainmenu-extension": "portal:/home/krassowski/jupyterlab/packages/mainmenu-extension",
+    "@jupyterlab/markdownviewer": "portal:/home/krassowski/jupyterlab/packages/markdownviewer",
+    "@jupyterlab/markdownviewer-extension": "portal:/home/krassowski/jupyterlab/packages/markdownviewer-extension",
+    "@jupyterlab/markedparser-extension": "portal:/home/krassowski/jupyterlab/packages/markedparser-extension",
+    "@jupyterlab/mathjax-extension": "portal:/home/krassowski/jupyterlab/packages/mathjax-extension",
+    "@jupyterlab/mermaid": "portal:/home/krassowski/jupyterlab/packages/mermaid",
+    "@jupyterlab/mermaid-extension": "portal:/home/krassowski/jupyterlab/packages/mermaid-extension",
+    "@jupyterlab/metadataform": "portal:/home/krassowski/jupyterlab/packages/metadataform",
+    "@jupyterlab/metadataform-extension": "portal:/home/krassowski/jupyterlab/packages/metadataform-extension",
+    "@jupyterlab/metapackage": "portal:/home/krassowski/jupyterlab/packages/metapackage",
+    "@jupyterlab/nbconvert-css": "portal:/home/krassowski/jupyterlab/packages/nbconvert-css",
+    "@jupyterlab/nbformat": "portal:/home/krassowski/jupyterlab/packages/nbformat",
+    "@jupyterlab/notebook": "portal:/home/krassowski/jupyterlab/packages/notebook",
+    "@jupyterlab/notebook-extension": "portal:/home/krassowski/jupyterlab/packages/notebook-extension",
+    "@jupyterlab/observables": "portal:/home/krassowski/jupyterlab/packages/observables",
+    "@jupyterlab/outputarea": "portal:/home/krassowski/jupyterlab/packages/outputarea",
+    "@jupyterlab/pdf-extension": "portal:/home/krassowski/jupyterlab/packages/pdf-extension",
+    "@jupyterlab/pluginmanager": "portal:/home/krassowski/jupyterlab/packages/pluginmanager",
+    "@jupyterlab/pluginmanager-extension": "portal:/home/krassowski/jupyterlab/packages/pluginmanager-extension",
+    "@jupyterlab/property-inspector": "portal:/home/krassowski/jupyterlab/packages/property-inspector",
+    "@jupyterlab/rendermime": "portal:/home/krassowski/jupyterlab/packages/rendermime",
+    "@jupyterlab/rendermime-extension": "portal:/home/krassowski/jupyterlab/packages/rendermime-extension",
+    "@jupyterlab/rendermime-interfaces": "portal:/home/krassowski/jupyterlab/packages/rendermime-interfaces",
+    "@jupyterlab/running": "portal:/home/krassowski/jupyterlab/packages/running",
+    "@jupyterlab/running-extension": "portal:/home/krassowski/jupyterlab/packages/running-extension",
+    "@jupyterlab/settingeditor": "portal:/home/krassowski/jupyterlab/packages/settingeditor",
+    "@jupyterlab/settingeditor-extension": "portal:/home/krassowski/jupyterlab/packages/settingeditor-extension",
+    "@jupyterlab/settingregistry": "portal:/home/krassowski/jupyterlab/packages/settingregistry",
+    "@jupyterlab/shortcuts-extension": "portal:/home/krassowski/jupyterlab/packages/shortcuts-extension",
+    "@jupyterlab/statedb": "portal:/home/krassowski/jupyterlab/packages/statedb",
+    "@jupyterlab/statusbar": "portal:/home/krassowski/jupyterlab/packages/statusbar",
+    "@jupyterlab/statusbar-extension": "portal:/home/krassowski/jupyterlab/packages/statusbar-extension",
+    "@jupyterlab/terminal": "portal:/home/krassowski/jupyterlab/packages/terminal",
+    "@jupyterlab/terminal-extension": "portal:/home/krassowski/jupyterlab/packages/terminal-extension",
+    "@jupyterlab/testing": "portal:/home/krassowski/jupyterlab/packages/testing",
+    "@jupyterlab/theme-dark-extension": "portal:/home/krassowski/jupyterlab/packages/theme-dark-extension",
+    "@jupyterlab/theme-dark-high-contrast-extension": "portal:/home/krassowski/jupyterlab/packages/theme-dark-high-contrast-extension",
+    "@jupyterlab/theme-light-extension": "portal:/home/krassowski/jupyterlab/packages/theme-light-extension",
+    "@jupyterlab/toc": "portal:/home/krassowski/jupyterlab/packages/toc",
+    "@jupyterlab/toc-extension": "portal:/home/krassowski/jupyterlab/packages/toc-extension",
+    "@jupyterlab/tooltip": "portal:/home/krassowski/jupyterlab/packages/tooltip",
+    "@jupyterlab/tooltip-extension": "portal:/home/krassowski/jupyterlab/packages/tooltip-extension",
+    "@jupyterlab/translation": "portal:/home/krassowski/jupyterlab/packages/translation",
+    "@jupyterlab/translation-extension": "portal:/home/krassowski/jupyterlab/packages/translation-extension",
+    "@jupyterlab/ui-components": "portal:/home/krassowski/jupyterlab/packages/ui-components",
+    "@jupyterlab/ui-components-extension": "portal:/home/krassowski/jupyterlab/packages/ui-components-extension",
+    "@jupyterlab/vega5-extension": "portal:/home/krassowski/jupyterlab/packages/vega5-extension",
+    "@jupyterlab/workspaces": "portal:/home/krassowski/jupyterlab/packages/workspaces",
+    "@jupyterlab/workspaces-extension": "portal:/home/krassowski/jupyterlab/packages/workspaces-extension",
+    "@jupyterlab/testutils": "portal:/home/krassowski/jupyterlab/testutils",
+    "@jupyter/ydoc": "3.0.0",
+    "@types/react": "18.0.26",
+    "react": "18.2.0",
+    "yjs": "13.5.49",
+    "y-protocols": "1.0.5"
   }
 }
diff --git a/tsconfig.json b/tsconfig.json
index 40989c6..b2695bc 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,6 +1,7 @@
 {
   "compilerOptions": {
     "allowSyntheticDefaultImports": true,
+    "skipLibCheck": true,
     "composite": true,
     "declaration": true,
     "esModuleInterop": true,

@krassowski
Copy link
Member Author

Maybe you did not re-complie on the jupyterlab lab side?

@davidbrochart
Copy link
Collaborator

Maybe you did not re-complie on the jupyterlab lab side?

Yes I did, but your instructions here solved the issue for me. This is definitely not my zone of comfort 🤔

@davidbrochart
Copy link
Collaborator

Works fine on my side, and the RTC: prefix is gone 🎉

@davidbrochart
Copy link
Collaborator

One thing I'm wondering: I was expecting to be able to right click e.g. on a notebook and be able to open it with the RTC content provider or with the REST content provider, but it seems I have no choice but using RTC.

@krassowski
Copy link
Member Author

Yes, this is a choice we have, either:

  • register a new widget factory for notebook and allow users to choose (via "Open with" or in settings), or
  • overwrite the content provider on the default factory

I chose the latter for simplicity and to avoid user confusion. We can change it.

@davidbrochart
Copy link
Collaborator

I chose the latter for simplicity and to avoid user confusion.

I think we can start with that, as it was previously like that.
Actually there was a possibility to open a file in non-RTC, if it had been opened before jupyter-collaboration was installed, because it was in the workspace. But this led to a lot of confusion, and this PR makes it clearer because it will always be opened in RTC so it's even better 👍

* A Collaborative implementation for an `IDrive`, talking to the
* server using the Jupyter REST API and a WebSocket connection.
*/
export interface ICollaborativeDrive extends Contents.IDrive {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we'll want to log that somewhere in the changelog as API breaking change, since other extensions may currently depend on ICollaborativeDrive?

For instance it's the case of JupyterCAD and JupyterGIS:

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the transition will be smooth because:

  • both extensions used ICollaborativeDrive as an optional dependency (after all it was just added)
  • neither of the extensions actually relied on the Drive part of ICollaborativeDrive - the same properties that they needed will be available on ICollaborativeContentProvider

So when cutting the release we will just need to add a note that migration from ICollaborativeDrive to ICollaborativeContentProvider token for access to shared factory and forks will be required. I do not see a good way to do that right now as the changelog for next version is not there yet - is it ok if we add it during/just after cutting the release?

);
}
const registry = defaultDrive.contentProviderRegistry;
if (!registry) {
Copy link
Member

Choose a reason for hiding this comment

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

Will that make jupyter-collaboration only compatible with JupyterLab 4.4 or higher?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I would propose that, the next version of jupyter-collaboration will require JupyterLab 4.4.

@jtpio
Copy link
Member

jtpio commented Dec 30, 2024

Just tried the latest changes of this PR and jupyterlab/jupyterlab#17092 locally with a dev install and it seems to be working fine. Also when opening files with other packages like voila and notebook (just opening the link, notebook would still need to update to the latest packages:

(posting a screenshot below just for self-reference)

image

tsconfig.json Outdated Show resolved Hide resolved
@krassowski
Copy link
Member Author

It looks like there is no version pins on JupyterLab in the distribution tarballs. I opened #425 to discuss this.

@krassowski
Copy link
Member Author

All green 💚! Good to merge and release an alpha version for testing?

@krassowski
Copy link
Member Author

CC @jtpio @davidbrochart - if you do not have any objections I would like to proceed with a merge and new alpha release. I think this would be a major version to signify that it is not backwards compatible.

@jtpio
Copy link
Member

jtpio commented Jan 7, 2025

I just tried locally in a new environment with the latest JupyterLab 4.4.0a2 and the wheels from https://github.com/jupyterlab/jupyter-collaboration/actions/runs/12651599033?pr=418, and it seems to be working well 👍

@jtpio
Copy link
Member

jtpio commented Jan 7, 2025

it seems to be working well 👍

There seems to be an issue with "Online Collaborator" section of the collaborator panel disappearing when a collaborator opens a text file, not sure if it's related though:

image

Edit:

TypeError: s[0] is undefined
jupyter-collaboration-text-file-issue.webm

@krassowski
Copy link
Member Author

.txt files open fine for me, but I was able to reproduce this with the .toml file. I saw:

TypeError: Cannot read properties of undefined (reading 'icon')
    at collaboratorspanel.js:92:1
    at Array.map (<anonymous>)

So this is likely due to incorrect the non-null assertion in:

const fileTypes = props.docRegistry
?.getFileTypesForPath(path[1])
?.filter(ft => ft.icon !== undefined);
const icon = fileTypes ? fileTypes[0].icon! : fileIcon;

Since this is not introduced by this PR and should be easy to fix - I will open a new issue.

@krassowski
Copy link
Member Author

The issue to track the bug discovered above: #426

@krassowski krassowski merged commit 14c1488 into jupyterlab:main Jan 7, 2025
27 checks passed
@krassowski krassowski deleted the content-provider-v2 branch January 7, 2025 16:58
@jtpio
Copy link
Member

jtpio commented Jan 7, 2025

I think this would be a major version to signify that it is not backwards compatible.

Right, a new major release sounds like a safer approach 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTC breaks notebook links
3 participants