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

Remove modeldb #12695

Merged
merged 8 commits into from Sep 19, 2022
Merged

Remove modeldb #12695

merged 8 commits into from Sep 19, 2022

Conversation

dmonad
Copy link
Member

@dmonad dmonad commented Jun 13, 2022

This PR implements a lightweight alternative to #12359. As we discussed last year, we wanted to remove modeldb as it maintains the same information as shared-model. This PR does just that.

As we can remove modeldb, we can make the shared-models package the only source of truth, which will greatly simplify double-binding logic and can help us to avoid the initialization problem (concurrent initializations by different peers can lead to users overwriting content). There should be no syncing back-and-forth between, for example, the notebook widgets and shared models. When somebody wants to add a new widget, they need to modify the shared notebook model, which will trigger the same event handlers as if there would be a remote insertion of a new cell. This simplifies the codebase + we can throw out duplicate implementations of the same functionality (overall this PR removes 1300 lines of code).

The most important contribution of this PR is that it completely avoids the initialization problem. It is important for us that there is only one way to initialize content (via shared-models). The shared-model package initializes a document safely with a single cell. That means users don't have to wait for the server to respond before editing. This PR removes all initialization code that we previously used, which was fairly problematic in shared editing scenarios.

Future PRs could move the shared models defined here again into separate packages (e.g. into the cells package as #12359 does). Although I recommend keeping the shared state separate from the application-specific state (e.g. models containing dirty flag, mimeType, selections, and another application-specific state like the editor instance that renders the model). This also enables others to reuse the shared-models package.

Code changes

Throughout the codebase, there are many places where we follow a fairly complicated event flow. This PR simplifies the event flow and enforces that the shared model is the only source of truth for all editor content. This leads to a significant reduction in the number of lines as we can avoid a lot of the complicated "double-binding" logic (syncing one state with another).

This PR retains the double-binding logic for metadata (which is an ObservableMap). It gives users the ability to have fine-grained access to metadata that many depend on. Ideally, we move the metadata field into the shared-model package and remove the double-binding logic. This can be done in a future PR.

User-facing changes

None

Backwards-incompatible

We remove the modeldb API, which might be relevant to extension authors.


@hbcarlos @davidbrochart @SylvainCorlay @echarles

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@davidbrochart
Copy link
Contributor

Thanks @dmonad.
It looks like you need to rebase.

@echarles
Copy link
Member

Thx for working on this @dmonad. The intent as shown in the first note of this PR sounds clear to me an a good step forward.

I have quickly scanned the source, and the changes are bringing more clarity and simplification as expected. I am happy to dig more and bring concrete reviews depending on the evolution of the discussion happening in jupyterlab/frontends-team-compass#150 and jupyterlab/frontends-team-compass#149

I see a lot of calls like ...model.sharedModel... (eg editor.model.sharedModel.getSource()). At some point, we could think to add shortcut methods to get rid of that call and simply expose editor.model.getSource() if possible. The whole idea would be to have a model hosted in the shared-models package that can be access by the other packages.

This PR is an excellent initiative to have something concrete to discuss and take a decision on the next steps.

@SylvainCorlay
Copy link
Member

Many thanks to @fcollonval for completing this!

@fcollonval
Copy link
Member

Important note the memory leak job highlight that a notebook is not cleared from memory when closed. This should be fixed before merging.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

bot please run benchmarks

const newCell = this.widgets[to];
for (const state in viewModel) {
// @ts-expect-error Cell has no index signature
newCell[state] = viewModel[state];

Check warning

Code scanning / CodeQL

Prototype-polluting assignment

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from [library input](1).
dmonad and others added 4 commits September 15, 2022 13:33
fix typings

fix docregistry tests

fix cells tests

fix notebook tests

fix notebook tests (2)

make notebook tests run more reliably & faster

remove unused code

re-add new dependencies after rebase

fix completer tests

fix console tests

fix fileeditor tests

fix integrity tests

simplify and use default parameters for CellModels

remove rebase bug

package integrity updates

add sample to template

package integrity updates

add json representation to ymodel template

remove ysource from YDocument class

revert changes to staging/package.json
bump y-codemirror.next

Hide modelDB from public API

Fix search styling

Align with code style guiidelines

Set execution to null by default

More refactoring and dirty state fix

Fix lsp package

Fix unit tests

Fix integrity

Fix translation

Fix integrity

Preserve state when moving cells

Update Playwright Snapshots
@fcollonval
Copy link
Member

bot please update documentation snapshots

@fcollonval fcollonval merged commit 4b8f2de into jupyterlab:master Sep 19, 2022
hbcarlos pushed a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* deduplicate modeldb - cells,codeeditor,codemirror

fix typings

fix docregistry tests

fix cells tests

fix notebook tests

fix notebook tests (2)

make notebook tests run more reliably & faster

remove unused code

re-add new dependencies after rebase

fix completer tests

fix console tests

fix fileeditor tests

fix integrity tests

simplify and use default parameters for CellModels

remove rebase bug

package integrity updates

add sample to template

package integrity updates

add json representation to ymodel template

remove ysource from YDocument class

revert changes to staging/package.json

* Fix activeCellChanged may emit null

bump y-codemirror.next

Hide modelDB from public API

Fix search styling

Align with code style guiidelines

Set execution to null by default

More refactoring and dirty state fix

Fix lsp package

Fix unit tests

Fix integrity

Fix translation

Fix integrity

Preserve state when moving cells

Update Playwright Snapshots

* Fix following rebase

* Fix example

* Fix UI test

* Various fixes

* Fix integrity

* Update snapshots

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
hbcarlos pushed a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* deduplicate modeldb - cells,codeeditor,codemirror

fix typings

fix docregistry tests

fix cells tests

fix notebook tests

fix notebook tests (2)

make notebook tests run more reliably & faster

remove unused code

re-add new dependencies after rebase

fix completer tests

fix console tests

fix fileeditor tests

fix integrity tests

simplify and use default parameters for CellModels

remove rebase bug

package integrity updates

add sample to template

package integrity updates

add json representation to ymodel template

remove ysource from YDocument class

revert changes to staging/package.json

* Fix activeCellChanged may emit null

bump y-codemirror.next

Hide modelDB from public API

Fix search styling

Align with code style guiidelines

Set execution to null by default

More refactoring and dirty state fix

Fix lsp package

Fix unit tests

Fix integrity

Fix translation

Fix integrity

Preserve state when moving cells

Update Playwright Snapshots

* Fix following rebase

* Fix example

* Fix UI test

* Various fixes

* Fix integrity

* Update snapshots

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.