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

🪐 Support for JupyterLab 4.x (take 2!) #155

Merged
merged 59 commits into from
Jun 23, 2023

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jun 15, 2023

@rowanc1 this PR dispenses with the attempt to move everything into a MIME renderer. Following the work on #142, it's clear to me that there was a reasonable amount of trying to fit a square peg into a round hole.

We should squash the commits when merging, as their lack of meaning represents the haphazard way that I built it out.

In this PR there is still be "double rendering" when executing a Markdown cell with expressions. I think this is probably reasonable; we probably don't want to hang the UI whilst waiting for a long expression to render. Maybe others will have different expectations, in which case we can rethink the approach.

In this rework, we're back to a distinct cell-vs-mime pathway. This seems like a more natural fit, given that fragmented documents need a lot more work than a single-document renderer. From #142, each cell is given a MySTWidget that re-uses the React rendering code. Placeholders should now be supported; if a cell has a placeholder during render(), only the AST is parsed over a render.

I anticipate that the initial load of a notebook might need optimising; this involves an N^2 render, because all cells are updated. We could detect this with an initialRender flag, and defer document-level rendering until the final cell.

agoose77 and others added 30 commits June 11, 2023 15:27
* chore: simplify dependencies

* fix: update source-map-loader

Addresses error with nth-check source-map parsing: webpack-contrib/source-map-loader#186

* chore: upgrade to PyPI version
This is needed for a different PR! oops
…140)

* 🎨 Update for showing proofs

* Update Playwright Snapshots

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SHA256 hashes:

jupyterlab-myst-1.2.0.tgz: e730003c864ec43b85b1d2f0f8fe2cc9ad318a9d9caaa48348b055d2ee85f9d0

jupyterlab_myst-1.2.0-py3-none-any.whl: 29b8055003e713058f9702fcf8d28fbbf74002c9e66cf4625d8746d7ee8f22ae

jupyterlab_myst-1.2.0.tar.gz: fbc686a79543cdd439d256c71d26410d281bcdff6620ff92b7fea247a71b6f12
@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 22, 2023

I spent much of today digging into this but have very little to show. The double rendering is persistent even when I can't see anything that would change. I will continue to debug -- one place I have limited visibility is what the jupyterlab-react interface is doing, and if that is forcing rerenders somehow.

A rerender is fine as it should be pretty efficient, however the flash is not good.

There are a few other bugs reintroduced (images, md render error, tasks disabled) that I might move onto instead to get this closer to a merge.

@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 22, 2023

@agoose77 I spent another few hours this evening getting into the react loop, I really don't think it is us, I am pretty sure the react node is getting killed by Jupyter, leading to a flash. We can "fix" it by changing the timing on the delayed trigger from the markdown activityMonitor. I think changing that from 1000ms --> 100ms makes the double render mostly go away in my testing. What I think is really happening is that the flash is happening before you shift-enter, which looks better. This means if you have two views of the notebook open, it is not super great. I am really not sure what Jupyter is doing with the react connection here.

With that fix, the UX seems great. The code, less so. :)

That is my recommendation for now, and then otherwise all other functionality is good to go. Updated libraries and fixed a few small other issues. I also enabled abbreviations and terms/glossaries.

Let's fix the screenshot test after we squash this into main (otherwise we have to force-push, run the update, un-force push).

A mini-bug is that the task list is enabled in the markdown preview, and it doesn't do anything.

@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 22, 2023

I also pushed a change and now have tested in JupyterLab3. Everything works except for the cell execution, I also don't think that would be hard to make it compatible, but we don't have to do that out of the gate. :)

@agoose77 can you take a look & test and then we can release!

Copy link
Collaborator

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

🚀

@rowanc1 rowanc1 changed the title feat: support JupyterLab v4 (take 2!) 🪐 Support for JupyterLab 4.x (take 2!) Jun 22, 2023
@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 23, 2023

Slight improvements on metadata set/get/delete that is backwards compatible. Now everything except inline widgets are working in jlab3.x!

My plan is to squash, merge and release a major version in the next hour.

@rowanc1 rowanc1 merged commit 3eb5b7b into main Jun 23, 2023
@rowanc1 rowanc1 deleted the agoose77/feat-jupyterlab-v4-next branch June 23, 2023 19:30
@parmentelat
Copy link

I have taken the liberty to quote your code in this discourse question here, hope you don't mind

https://discourse.jupyter.org/t/jlab-extensions-that-target-both-jlab3-and-jlab4/20100

I was about to go down a similar route in order to support both jlab 3 and 4, so seeing your code prompted me to ask the question on behalf of all extension writers ;-)

and many thanks btw for releasing 2.0 🥇

@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 26, 2023

Nice -- looking forward to seeing the responses!

@parmentelat
Copy link

yeah, me too, insofar as I get any ;-)

"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
]
dependencies = [
"jupyter_server>=2.0.1,<3"
Copy link

Choose a reason for hiding this comment

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

Do we need jupyter_server as a dependency?

Copy link

Choose a reason for hiding this comment

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

This seems to be causing installation issue in JupyterLite, as reported in jupyterlite/jupyterlite#1122.

Copy link

Choose a reason for hiding this comment

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

Opened #177

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.

5 participants