packaging / dependency wrt reveal.js / publish a rise-reveal npm package

parmentelat commented 5 years ago

related to #475 and #381

this is an attempt at summarizing ideas raised in other issues

classic notebook

of course RISE depends on reveal.js; however it is not quite possible to use it as-is historically, we had a rather straightforward patch of reveal.css in place; as part of moving to reveal-3.8.0, the set of patches reached a little wider scope

regardless, what we do for packaging the whole thing is to locally npm install reveal.js, patch what needs to be patched, and repackage that as part of the static area of the RISE jupyter (classic) extension

also relevant, although maybe at the border of this current discussion, is the case of reveal's plugins; we have support for 2 such plugins:

notes for supporting a speaker view
chalkboard for adding free-hand drawings on top of the slideshow

at this point, IIUC but Damian please confirm/infirm, what we've done about these 2 extensions is:

notes: I can see a source subdir plugin/notes which, in my understanding again, is a derived copy of reveal's note plugin;
- not quite sure at this point how this evolves when upgrading to a newer reveal
- another thing to confirm is, the sources for the notes plugin ship with reveal, right ?
chalkboard - this one I know does not ship with reveal
- we use the plugin as-is
- so we've made a PR with the original author of the chalkboard plugin, so that the package can be published on npm (and IIUC I did the only publication myself)
- bottom line is that RISE just needs to add that dependency and we're done as far as packaging is concerned

jupyterlab extension

I've just only started to look into that; the matter being complicated by the fact that 1.0 just came out, plus 0.35 and 1.0 come with a (slightly) different framework.

regardless, it seems like for the sake of an easy packaging for the jlab extension, having an npm package of rise-reveal - that is to say, RISE's variant of reveal.js - that would come with all the patches, would be just right

and that package would make a lot of sense with the classic RISE as well AAMOF, even if the patches required for both variants (classic and jlab) may differ

thoughts and discussion

first off, is that summary accurate ?
- and if so what about the notes thing ? (since chalkboard can remain as it is now)
what's best in that case:
- 1 or 2 git repos (for classic and jlab) ?
- 1 or 2 npm packages (rise-reveal-classic and rise-reveal-jlab) ?
npm versioning scheme: we should have something that reflects both reveal internal version (like e.g. 3.8.0) and our local numbering; maybe use 380.1.0[.dev*n*]; does that even make sense ?

so yes, food for thought :)

parmentelat commented 5 years ago

I have had a first experience with jlab's devel tools; for the record I tried to use relative npm dependencies like this

  "dependencies": {
    "@jupyterlab/application": "^1.0.0-rc.0",
    <snip>,
    "rise-reveal": "file:../rise-reveal"
  },

in an attempt to avoid having to publish rise-reveal up to npm each time there's a change - because at first in devel mode that would make the workflow super slow

I have not been successful with that so far, I wonder if that's supported

parmentelat commented 5 years ago

At that point my proposal, to answer some of the questions above, would be

one single git repo; with 3 parts named classic, rise-reveal, and jlab
one single rise-reveal npm package; I still don't know if the patches to reveal need to differ for classic and jlab, but in any case we can always expose reveal-classic.css and reveal-jlab.css so it seems overkill to have 2 separate npm packages

damianavila commented 5 years ago

This is exciting. I have a lot of thoughts and some useful feedback... I will try to reply to you tonight/early tomorrow.

parmentelat commented 5 years ago

I had a favorable timeslot recently so I went ahead :)
You'l find a working sketch on the split-rise-reveal branch in your repo;
I took that chance to do quite some cleanup, and maybe some changes will sound disruptive, as always feel free to speak up if that's the case

damianavila commented 5 years ago

not quite sure at this point how this evolves when upgrading to a newer reveal

We should check the version in the newer reveal.js and see if there are any significant changes compared with the previous version. The notes plugin we ship by ourselves is just a patched version of the notes plugin coming by default in reveal.js itself. The changes are minimal but important to be able to show the slideshow in the notebook plugin.

damian@DESKTOP-LP3SME9:~/devel/foo$ diff notes_rise.js notes_reveal.js
11,13d10
<  *
<  * Modified version from 3.5.0 notes plugin used by RISE
<  *
20c17
<                       var jsFileLocation = document.querySelector('script[src*="notes.js"]').src;  // this js file path
---
>                       var jsFileLocation = document.querySelector('script[src$="notes.js"]').src;  // this js file path
145c142
<                       if( event.keyCode === 84 ) {
---
>                       if( event.keyCode === 83 ) {

damianavila commented 5 years ago

regardless, it seems like for the sake of an easy packaging for the jlab extension, having an npm package of rise-reveal - that is to say, RISE's variant of reveal.js - that would come with all the patches, would be just right

I agree, and that was what I have started to do in that prototype PR when I publish the patched reveal.css as reveal_rise. But I guess we can end up with a custom RISEd reveal.js with all the patches we need, for instance, the patches in the notes pluggin.

and that package would make a lot of sense with the classic RISE as well AAMOF, even if the patches required for both variants (classic and jlab) may differ

I agree too.

damianavila commented 5 years ago

first off, is that summary accurate ?

Yep

and if so what about the notes thing ? (since chalkboard can remain as it is now)

I explain that above.

what's best in that case: 1 or 2 git repos (for classic and jlab) ?

I do not think so. Let's just keep everything in one repo.

1 or 2 npm packages (rise-reveal-classic and rise-reveal-jlab) ?

Makes sense to me.

npm versioning scheme: we should have something that reflects both reveal internal version (like e.g. 3.8.0) and our local numbering; maybe use 380.1.0[.devn]; does that even make sense ?

mmm... 380.1.0 feels weird... what about some file with the reveal version we inherit from inside the package and we use our own version number starting from 1.0.0?

damianavila commented 5 years ago

I have not been successful with that so far, I wonder if that's supported

Not sure if it is supported, when I tried something like that in the past, it did not work, so I ended up publishing stuff to npm.

damianavila commented 5 years ago

one single git repo; with 3 parts named classic, rise-reveal, and jlab

Great, we agreed :wink:

one single rise-reveal npm package; I still don't know if the patches to reveal need to differ for classic and jlab, but in any case we can always expose reveal-classic.css and reveal-jlab.css so it seems overkill to have 2 separate npm packages

Sounds good to me!

damianavila commented 5 years ago

I had a favorable timeslot recently so I went ahead :)

Great!

You'l find a working sketch on the split-rise-reveal branch in your repo;

I will take a look.

I took that chance to do quite some cleanup, and maybe some changes will sound disruptive, as always feel free to speak up if that's the case

OK, can you make a PR of that branch so I can comment on it and we can discuss it?

parmentelat commented 5 years ago

about versioning, IMHO it is key that the release number of rise-reveal clearly states on what version of reveal.js this is based on I'm not sure how to use semver to reflect both that and our own progress
FYI the (single) package that I published on npm follows the scheme that I proposed above

damianavila commented 5 years ago

about versioning, IMHO it is key that the release number of rise-reveal clearly states on what version of reveal.js this is based on

Can I ask why a reference in a file is not enough?

I'm not sure how to use semver to reflect both that and our own progress

Not sure either.

FYI the (single) package that I published on npm follows the scheme that I proposed above

That's OK for now, but not sure if we should adopt that pattern in the future.

damianavila / RISE