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

Add a plugin to be able to swap the doc provider #10256

Merged
merged 17 commits into from May 20, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented May 17, 2021

References

Fixes #10252

Early draft for what it would look like to have a plugin that can be used to set custom providers.

TODO

  • Move the doc provider instantiation to a plugin
  • Rename IProvider to IDocumentProvider
  • Docstrings for the IDocumentProvider interface
  • Move reading the page config option to the plugin?
  • Create a docprovider-extension package?

Code changes

Move the creation of the provider in the document context to a plugin, which can then be disabled or replaced in other lab extensions and distributions.

User-facing changes

None

Backwards-incompatible changes

Add an optional parameter to some of the instantiation options.

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jtpio
Copy link
Member Author

jtpio commented May 17, 2021

Rebased to grab #10258, so this PR can be tested on Binder with RTC enabled.

@echarles
Copy link
Member

@jtpio quick feedbacks

  • The token IDocumentProviderFactory defined in docprovider is provided with a plugin const docProviderPlugin: JupyterFrontEndPlugin<IDocumentProviderFactory> = { in the docmanager-extensionpackage. Is that inline with jupyterlab architecture? I mean, I was expected to see a new docprovider-extension package to do that job. A new package for that single bit may sound overkill, but it is like that in many other existing packages.

  • What about docregistry. For now, the IDocumentProviderFactory is passed to the registry through options. Wouid it make sense to also inject it? In that case we would need to convert the docregistry to an extension, which is not the case now.

@jtpio
Copy link
Member Author

jtpio commented May 18, 2021

  • I was expected to see a new docprovider-extension package to do that job. A new package for that single bit may sound overkill, but it is like that in many other existing packages.

Yes, it's still one of the remaining TODO items listed in #10256 (comment).

This was also discussed yesterday during the RTC call (https://hackmd.io/UbnBH58hS8itoWgfiWT77A) and the conclusion was that it would indeed make sense to have a docprovider-extension package.

I can update this PR to add it.

@jtpio
Copy link
Member Author

jtpio commented May 18, 2021

  • What about docregistry. For now, the IDocumentProviderFactory is passed to the registry through options. Wouid it make sense to also inject it? In that case we would need to convert the docregistry to an extension, which is not the case now.

Not sure yet. But probably something to think about as a follow-up?

@echarles
Copy link
Member

I can update this PR to add it.

Great!

Not sure yet. But probably something to think about as a follow-up?

Looking more at the constructs, the Context is created by the docmanager which is injected with the correct IDocumentProviderFactory, so having an docregistry-extension does not make sense for that case.

Another enhancement would be to make the docregister/context a bit more agnostic, so not having to depend directly on the yjs library, so not having to maintain those 2 fields

  private _ydoc: Y.Doc;
  private _ycontext: Y.Map<string>;

@github-actions github-actions bot added Design System CSS tag:CSS For general CSS related issues and pecadilloes labels May 18, 2021
@jtpio
Copy link
Member Author

jtpio commented May 18, 2021

The splice_source check fails because docprovider-extension is not published to npm yet.

@jtpio jtpio marked this pull request as ready for review May 18, 2021 15:15
@jtpio
Copy link
Member Author

jtpio commented May 18, 2021

Added the docprovider-extension package (cc @echarles if you want to have a look).

Some CI checks will fail until the package is published to npm.

@echarles
Copy link
Member

I have quickly scanned the diff and it LGTM. Do you have more to add before I review?

@jtpio
Copy link
Member Author

jtpio commented May 18, 2021

Thanks.

I think it should be ready to go.

Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

Reviewed and tested. LGTM. Let's wait 2 days before merging to get any further comment.

@jtpio
Copy link
Member Author

jtpio commented May 20, 2021

Thanks @echarles.

A follow-up to this PR would be to publish the new docprovider-extension package to npm to fix the failing CI checks.

@blink1073
Copy link
Member

Thanks @jtpio and @echarles! I'll cut another alpha with this today (in a few hours), but will hold off merging until then so CI stays working.

@blink1073 blink1073 merged commit 4bd902c into jupyterlab:master May 20, 2021
@blink1073
Copy link
Member

Releasing now

@blink1073
Copy link
Member

https://github.com/jupyterlab/jupyterlab/releases/tag/v3.1.0a9

@jtpio jtpio deleted the docprovider-plugin branch May 20, 2021 21:42
@echarles
Copy link
Member

Thx @jtpio and @blink1073

cameron-toy pushed a commit to cameron-toy/jupyterlab that referenced this pull request May 28, 2021
* Add plugin to create an IProviderFactory

* Fix typo in tsconfig.json

* Fix plugin id

* Lint

* Rename IProvider to IDocumentProvider

* Read PageConfig collaborative from the plugin

* Integrity

* Provider URL is specific to websocket

* Consistent WebSocket spelling

* Add docprovider-extension package

* Remove schemaDir

* Remove TODO

* Update sideEffects

* Update plugin id

* Add docprovider-extension to SKIP_CSS

* Add docstrings

* More docstrings
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Nov 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design System CSS enhancement pkg:docmanager pkg:docprovider pkg:docregistry status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes tag:Real Time Collaboration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pluggable provider for real time collaboration
3 participants