-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
* 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
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. |
@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 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. |
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! |
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.
🚀
Slight improvements on metadata set/get/delete that is backwards compatible. Now everything except inline widgets are working in My plan is to squash, merge and release a major version in the next hour. |
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 🥇 |
Nice -- looking forward to seeing the responses! |
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" |
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.
Do we need jupyter_server
as a dependency?
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.
This seems to be causing installation issue in JupyterLite, as reported in jupyterlite/jupyterlite#1122.
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.
Opened #177
@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 duringrender()
, 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.