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 binder... and entirely too much else... #10

Merged
merged 31 commits into from
Nov 11, 2020

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Nov 2, 2020

Hi! This is a cool looking project!

First off: sorry for this ridiculous PR. In the original scope, I just wanted to push a nice, clean little binder demo PR with an arguably less-hacky way to get the wasm to work (originally from here... we should probably get it into notebook, jupyter_server, or jupyterlab_server so nobody else has to deal). However, going pip-first is going to be great with JupyterLab 3, so it would probably be good to grab that package name,...

But in thinking about the even larger collection of plugins myst/jupyter-book would need, it seemed interesting to see what a multi-plugin setup would look like, where the "core" just provided the renderer, and each plugin did its own thing, and could even be extended by other extensions. But this lead to finding things that are supported in the marked renderer today, but missing in vanilla markdown-it, which would be table stakes for taking a real shot at getting something like this into jupyterlab core...

A couple hours later, sure, the binder works, but the remaining code is barely recognizable as i cargo culted things from other extensions.

Not sure how you'd like to proceed, but certainly give the binder a spin!

cc @choldgraf @tonyfast @goanpeca @jasongrout

@agoose77
Copy link
Owner

agoose77 commented Nov 2, 2020

Hi @bollwyvl,

Thank you very much for creating this PR. 🦸 I created this extension for my own uses during a time when I really oughtn't to have been working on side projects at all 🤣. Overall, I like the feel of these changes. In particular, eliminating the host mimetypes dependency is a clever trick - when I originally wrote the extension I was unfamiliar with server extensions, and the thought had not occurred to me since that time to monkey-patch the guesser ✨.

RE the plugin API, I was mulling this over myself because, on the one hand it seems like a lot of development burden for people who just want to load an existing md-it plugin, but on the other, if we just leverage requirejs then those extensions are not bundled statically (and increase the loading overhead?). I suppose that it is no bad thing if we have the main plugins bundled in core.

In terms of the design, I'm not an expert in JS - is it good practice to monkey-patch the md renderer instead of creating a new one?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 2, 2020 via email

@agoose77
Copy link
Owner

agoose77 commented Nov 2, 2020

@bollwyvl I am really looking forward to the Lab3 mechanism as it will really improve some of the UX issues I've had as both a developer and user of extensions :)

I think I've come across the issues you recall whereby the MD renderer wasn't being used all of the time.

Overall I'm very happy with this PR, I'll merge it for now because I think it's a significant improvement over where we are currently.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 2, 2020

Feel free to point me to anything you don't like in particular on this PR, happy to help move it forward! Off the top of my head:

probably in this pr:

  • a link to the binder in README
  • bump version
    • a full major bump seems reasonable :P

probably in a follow-on (this doesn't need to get any more complicated, unless you want it to):

  • handle some additional edge cases that i didn't cover
    • image backgrounds can have preferences
    • script tags (gaaaahh)
  • validate behavior with some "important" extensions that muck about with markdown, e.g. jupyterlab-toc
  • a preferences schema that allowed for:
    • turning off unwanted plugins, e.g. "disabled-plugins": ["whatever"]
    • or disabling the monkeypatch altogether to see what a document looks like with marked
      • if doing automated testing (with e.g. robotframework), this could assess the actual DOM differences without rebuilding
    • probably a minimal ui for config
      • extend IPluginProvider with a title and description, and maybe an examples for syntax help

out of scope:

  • releasing the serverextension on pypi :P
    • there's also a way to add some breadcrumbs for the extension manager in package.json/jupyter/lab (which could be on this PR)
      • but maybe who cares, as lab3 is where it's going? maybe not even release this for lab2?

@agoose77
Copy link
Owner

agoose77 commented Nov 2, 2020

Yes, agreed on ditching JLab2; once JLab 3 is released I'll upgrade all of the extensions I use on a daily basis where I can :)

Your list of issues looks like a great place to start. I'd suggest the separation into a new PR as you mention :)

One additional feature that might be a goal for the follow-up PR is allowing this parent extension to configure the plugins by passing in an options object. I don't know if it is considered reasonable to pass non-schema-backed configurations from JLab configuration to the consumer, but it would mean that users could easily configure things.

@agoose77
Copy link
Owner

agoose77 commented Nov 4, 2020

Also, @bollwyvl is there a reason that you're not included in the LICENSE file?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 4, 2020

not included in the LICENSE file?

I'm good... happy to do grunt work, if it enables others to do more with less work... and handle on-going maintenance! But seriously, I think the big goal is to mature this to where it could be considered for upstreaming into core. It's a lot easier to make the case with a robust, thought-out, already-works-today implementation than a long-running PR (oops).

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 4, 2020

Pushed with a fix to anchor URLs, verified while watching binder load.

Also added:

  • quick test of display to notebook (works)
  • added @jupyterlab/toc to binder (looks good locally)
    • though toc eats the pilcrow 😢
  • package/binder badges to readme (mostly broken links for now

I'd say kick the tires once more, then we're ready to rock, think about how to proceed.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 4, 2020

Yeah, binder looks good... toc doesn't add numbers to markdown files with either renderer, so not-bollwyvl's-problem...

@choldgraf
Copy link

This looks great to me 👍

src/settings.tsx Show resolved Hide resolved
src/plugin.ts Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
@agoose77
Copy link
Owner

agoose77 commented Nov 5, 2020

These are some really cool changes - I've not delved into the VDom support yet, so it is nice to see its utility in a larger extension. Overall, I'm really happy with this PR - I think this lays the groundwork for a really positive addition to the JLab ecosystem.

One thing I thought about when writing this extension is the current state of maths rendering - right now we have JLab extensions to change the maths renderer. This extension creates an additional extensibility layer one level lower, in the MD renderer itself via md-it plugins. However, right now we still defer to JLab for maths processing, so md-it cannot influence this stage. Should we move to an entirely md-it implementation and leverage md-it plugins for md rendering? This would require creating an additional MathJax + KaTeX extension.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 5, 2020

VDom support

I'm no great fan of react (and actively dislike jsx, as i carry many scars from CFXML), but it's the best shipped thing we have for leaf nodes since the Licensing Fiasco was resolved, and likely not to be going anywhere. the lumino/vdom stuff was also fine, but nothing in particular works with it (even the built-in lab stuff). but you can read it in an afternoon, vs react which is like a municpal water supply... few really understand how it works, but you all have to share one: when it works, its fine. when it breaks the whole city shuts down.

such a panel should probably be several lumino widgets (nav, sidebar, etc). but the scope of this just isn't large enough to warrant it, even though that file's getting pretty long. the win there would be, for example, each of the examples could be a notebook cell, which one could drag directly into notebook.

MathJax + KaTeX extension.

not poking that particular bear on this PR! that any latex works (like the link replacing) gives me cause for joy. people get real particular about their maths, so i haven't even looked at katex, and i don't know at whose feet it lies to document the inconsistencies... not me!

that being said, as some folks don't use math, and it does cause rendering hiccups, adding another layer of options to allow disabling math typesetting altogether would lie on this extension. this would allow a plugin to add their own and not have them fight, without putting the burden here. frankly, i'd like to see an extension that showed the performance (which would be bad) vs fidelity (which would be perfect) of deferring to a backend renderer, serving (cached), clean-as-the-driven snow SVG... for certain folks, this is the only real answer, as they have all their special macro packages, etc. for their field that nobody is going to take the time to port to js.

@agoose77
Copy link
Owner

agoose77 commented Nov 5, 2020

I hadn't really thought about the different VDom implementations that are out there, interesting. Probably out of discussion scope for this PR, but I'm sure you'll hear more questions from me on the subject.

i'd like to see an extension that showed the performance (which would be bad) vs fidelity (which would be perfect) of deferring to a backend renderer, serving (cached), clean-as-the-driven snow SVG... for certain folks, this is the only real answer, as they have all their special macro packages, etc. for their field that nobody is going to take the time to port to js.

This is actually a fascinating idea. There are several packages that I'd like but can't use at the moment... and a really dumb implementation wouldn't be that hard at all using processes. Food for thought ... 🔥

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 5, 2020 via email

@choldgraf
Copy link

So this conversation is now in a language that I no longer understand, but FWIW I support both of you continuing to be wizards and doing whatever you think is best 😅

@agoose77
Copy link
Owner

agoose77 commented Nov 5, 2020

I don't think a tectonic renderer would need to include any metadata about packages - doesn't it support lazy loading of packages just like TeXLive does? (I only briefly read the docs).

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 6, 2020

doesn't it support lazy loading of packages

oh and how! i trust tex stuff more than, say, unpkg/npm/requirejs, but am not sure why... but it's definitely another source of irreproducibility, or at least bewildering initial delay.

@jasongrout
Copy link

react which is like a municpal water supply... few really understand how it works, but you all have to share one: when it works, its fine. when it breaks the whole city shuts down.

I had to chuckle with your analogies here.

@agoose77
Copy link
Owner

agoose77 commented Nov 6, 2020

doesn't it support lazy loading of packages

oh and how! i trust tex stuff more than, say, unpkg/npm/requirejs, but am not sure why... but it's definitely another source of irreproducibility, or at least bewildering initial delay.

If we require users to install a separate extension to handle "true" LaTeX rendering, I think a disclaimer about initial package-load delays should be fine. In my experience using LaTeX distributions, the automatic package management approach has been a far more user friendly experience.

Also, whilst the information declaring which md-it plugins are required to render a notebook are not stored in the markup, but instead in the notebook metadata, it should not constrain future modifications / improvements to the specification. Understandably we want to get this right, but I think that would give us more leeway vs needing to anticipate ahead :)

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Nov 6, 2020

Yeah, just-in-time resolution-of-gigabytes-from-the-internet is great until.... it isn't. I'm in the pip/conda/apt install foo camp: take as long as you want, let me check in a fully-realized solution description from sources I know and trust (if even unfounded), and keep working when i get on a plane (remember when we got on planes?), and be able to reproduced on another machine at a later date.

ANYHOOOO: actionable takeaway of the above is:

  • the core extension can (but not on this PR, folk think it's worth it):
    • allow disabling built-in math typesetting (and or other features, like link replacement)
    • provide access to lifecycle hooks to plugins for when those features would have happened
  • a separate repo/extensoon entirely, can:
    • make use of the above
    • add a dedicated REST API (or perhaps, websocket, if it might be minutes before it "succeeds")
    • configurably source rendering from wherever it needs to in a thread
    • provide same at nbconvert time

@agoose77
Copy link
Owner

agoose77 commented Nov 6, 2020

Yeah, just-in-time resolution-of-gigabytes-from-the-internet is great until.... it isn't. I'm in the pip/conda/apt install foo camp: take as long as you want, let me check in a fully-realized solution description from sources I know and trust (if even unfounded), and keep working when i get on a plane (remember when we got on planes?), and be able to reproduced on another machine at a later date.

ANYHOOOO: actionable takeaway of the above is:

  • the core extension can (but not on this PR, folk think it's worth it):

    • allow disabling built-in math typesetting (and or other features, like link replacement)
    • provide access to lifecycle hooks to plugins for when those features would have happened
  • a separate repo/extensoon entirely, can:

    • make use of the above
    • add a dedicated REST API (or perhaps, websocket, if it might be minutes before it "succeeds")
    • configurably source rendering from wherever it needs to in a thread
    • provide same at nbconvert time

Hehe, I think that's where having a global package cache becomes a "feature" :P

I think your points summarise the issue nicely :)

@choldgraf
Copy link

Can I make a recommendation that we triage this PR's comments for some issues to create as future discussion / action points, and then get this PR shipped? :shipit:

:-)

@agoose77
Copy link
Owner

agoose77 commented Nov 9, 2020

Can I make a recommendation that we triage this PR's comments for some issues to create as future discussion / action points, and then get this PR shipped? :shipit:

:-)

I think @bollwyvl has already managed to do that :) I've moved my further discussion there now :)

@agoose77
Copy link
Owner

agoose77 commented Nov 11, 2020

I think this PR as-is is worth merging. The additional points of discussion, such as supporting codemirror extension need to be addressed before making a new release, but I'd prefer to do that from a new PR.

Another big thank-you to all contributors, especially @bollwyvl, for the work in this PR.

@agoose77 agoose77 merged commit 4f65520 into agoose77:master Nov 11, 2020
@choldgraf
Copy link

🎉🎉🎉🎉 this is very exciting :-) another step closer to much-improved markdown in JupyterLab!

@agoose77
Copy link
Owner

@choldgraf if you're interested, I've added some alpha versions to PyPI and npm so that you can install it as described in the README.

I'm waiting on JupyterLab 3.0 to support installing only via the PyPI package.

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

Successfully merging this pull request may close these issues.

5 participants