The Opal runtime dependency problem with Asciidoctor.js and other plugins

[ggrossetie]

I don't think think this project should have a direct dependency on asciidoctor.js. Instead, we should only declare asciidoctor.js as a peer dependency.

We'll need to document the minimum version of asciidoctor.js a given Asciidoctor Reveal.js release supports. We could do that through the peer dependency, or we can simply document it in the README.

We'll also need to update the install instructions and the development instructions since we will need to manually install the asciidoctor.js dependency.

We are also making this change in the CLI: https://github.com/asciidoctor/asciidoctor-cli.js

[obilodeau]

While we are in breaking changes mode, I'll put this in our next release as well.

I'll also plug the fact that asciidoctor-cli.js exists, I didn't know until now and it will be more familiar to users than telling them they need to write Javascript code.

[obilodeau]

At first, I started with a loose requirement on asciidoctor.js version but got this stack trace:

$ node ad.js 
/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:140
      Object.defineProperty(object, name, {
             ^

TypeError: Object.defineProperty called on non-object
    at Function.defineProperty (<anonymous>)
    at $defineProperty (/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:140:14)
    at Object.Opal.allocate_module (/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:546:5)
    at Opal.module (/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:593:19)
    at /home/olivier/src/asciidoc/test-npm/node_modules/asciidoctor-reveal.js/dist/main.js:33:41
    at /home/olivier/src/asciidoc/test-npm/node_modules/asciidoctor-reveal.js/dist/main.js:38:7
    at Opal.modules.asciidoctor-revealjs/converter (/home/olivier/src/asciidoc/test-npm/node_modules/asciidoctor-reveal.js/dist/main.js:39:5)
    at Object.Opal.load (/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:2298:7)
    at $Object.Opal.require (/home/olivier/src/asciidoc/test-npm/node_modules/opal-runtime/src/opal.js:2326:17)
    at /home/olivier/src/asciidoc/test-npm/node_modules/asciidoctor-reveal.js/dist/main.js:5064:10

I think this is due to a mismatch between the code generated by Ruby Opal versus what Asciidoctor.js Opal runtime is. Investigating more.

[obilodeau]

Put an explicit version dep in asciidoctor-reveal.js's package.json, then installed it, then with:

npm i asciidoctor.js@1.5.6

It worked but without the @1.5.6 it doesn't.

So what is the use of the peerDep if it can't put a proper restriction, I'm confused here.

It implies we will need to adjust the README everytime we adjust our opal or asciidoctor.js dependency.

Also, someone seeing our README (from master) might get confused if we bumped our dependency for development but the last packaged version dependency requires an older version.

Am I too old school here and seeing issues that don't exist?

[obilodeau]

Test branch for reference: https://github.com/asciidoctor/asciidoctor-reveal.js/compare/master...obilodeau:asciidoctorjs-peer-dependency?expand=1

[ggrossetie]

It worked but without the @1.5.6 it doesn't.

Meaning that Asciidoctor.js 1.5.7 is not compatible with Asciidoctor Reveal.js (master).

I think this is due to a mismatch between the code generated by Ruby Opal versus what Asciidoctor.js Opal runtime is. Investigating more.

Yes, most likely :disappointed: I'm using an unreleased version of Opal that contains fix for Asciidoctor.js: https://github.com/Mogztter/opal-node-runtime/releases/tag/v1.0.11 (built against: https://github.com/opal/opal/commit/6703d8d2).

It's really hard to predict when a new version of Opal will break "compile" compatibility. But it looks like Asciidoctor.js 1.5.6 is compatible with Opal 0.11.1 and Asciidoctor.js 1.5.7 is only compatible with Opal master (unreleased).

I think you don't even have a warning when you install asciidoctor.js 1.5.7 (ie. when you provide the peer dependency) because it's a patch version. So npm assumes that version 1.5.6 and 1.5.7 should work the same... Hopefully when we switch to semantic versioning it will make things easier.

Can we use an unreleased version as a Gem dependency ? https://github.com/asciidoctor/asciidoctor-reveal.js/blob/master/asciidoctor-revealjs.gemspec#L46

[obilodeau]

Ok, it worked with the pinned Gem but I'm wondering if the cure isn't worse than the disease... Let me explain:

I'm a user, I don't care about what version of asciidoctor-revealjs or asciidoctor.js I run. All I care is that my slides convert properly.

Now, we build our node package with a specific opal, and we need the matching runtime opal, which is out of this project's dependency list. So two different projects need to sync: asciidoctor.js' latest release, will require me to do a release with matching opal. More work for me. Requests for releases when I might not be able to deliver them due to other personal constraints.

Plus we are saying: asciidoctor.js is a peer dependency, so more work for the user (one more line to type, to be specific).

All this for the use-case that someone might re-use the same asciidoctor.js across presentations (and thus would save a little disk space and install time).

I might be wrong, or there's some advantage I might not see. Right now, all I'm thinking is confused colleagues saying:

I cp'ed your deck and re-ran installation instructions, and it no longer converts...

I propose that, given the current [Opal] constraints, asciidoctor.js is currently a direct upstream dependency of asciidoctor-reveal.js, and, as such, we cannot promote it to a peer dependency. Otherwise this is going to be harder on users and on this project (tracking opal revs, forced releases after asciidoctor.js releases).

Am I mistaken here? Because I definitely could. But, as Tron taught me, I fight for the users!

[mojavelinux]

You raise valid concerns, and I love that you fight for the users.

I just want to clarify one point for the discussion:

All this for the use-case that someone might re-use the same asciidoctor.js across presentations (and thus would save a little disk space and install time).

I don't think that's the objective of a peer dependency. The reason for a peer dependency is to allow the user to choose which version of the library they want to use, and have control over installing it within the project. It's used in cases when it's only a loose dependency (hence peer). While it's true that asciidoctor-reveal.js needs to use Asciidoctor/Asciidoctor.js, it doesn't need any specific one (once we get things stabilized). And thus we give the user that flexibility.

In short, a converter uses asciidoctor.js, but it's not responsible on bringing it (because there could be other plugins/extensions that use asciidoctor.js too).

[ggrossetie]

Dan is right but until we use semantic versioning I think it's better to use a direct dependency. As mentioned in my previous comment peerDependency works best when the library use semantic versioning.

[mojavelinux]

Sounds like a plan.

[obilodeau]

Sounds good. Targeting this move to a peerDependency for later.

As a reminder for myself later. Even under semantic versioning, we will need to be careful with Asciidoctor.js' opal-runtime requirement and this project's opal build-time dependency. They will need to be compatible. Hopefully Opal will have stabilized then.

Another note to self: the code is here: https://github.com/obilodeau/asciidoctor-reveal.js/tree/asciidoctorjs-peer-dependency, build failed because I forgot to add the asciidoctor.js installation in travis

[obilodeau]

@Mogztter I was thinking of closing this due to the way we now specify the Asciidoctor.js installation in the README and the fact that asciidoctor.js is now a devDependencies and not a dependency.

However, are we really handling the problem of Opal versions mismatch? If, in the future, you release Asciidoctor.js 2.1.0 (or 3.0.0) which requires a new Opal git rev, the README instructions here will no longer work. @asciidoctor will require Opal version X, @asciidoctor/reveal.js will be built using incompatible version Y.

I gave this some thought and since this would satisfy mixing plugins (like diagram and reveal.js) it would be nice to handle this cleanly for 3.0.

Until Opal stabilizes (or figures how to mix generated code from different versions), I don't see a long term solution besides making asciidoctor.js a direct dependency of this project. I understand that this can make integration with other Opal-compiled Asciidoctor.js plugins impossible (although they would most likely not work either).

So I thought of an alternate satisfactory solution: we should specify the version in the npm install string in the README assuming upstream asciidoctor.js guarantees that it will stick to the same Opal build for anything besides a major release. Something like:

npm i --save asciidoctor@^2.0 @asciidoctor/reveal.js

But I would like to see that agreement of not breaking things somewhere in the Asciidoctor.js repo.

[ggrossetie]

However, are we really handling the problem of Opal versions mismatch? If, in the future, you release Asciidoctor.js 2.1.0 (or 3.0.0) which requires a new Opal git rev, the README instructions here will no longer work. @asciidoctor will require Opal version X, @asciidoctor/reveal.js will be built using incompatible version Y.

That's true but I will release a breaking/major version of Asciidoctor.js when a new version of Opal is not binary compatible.

So I thought of an alternate satisfactory solution: we should specify the version in the npm install string in the README assuming upstream asciidoctor.js guarantees that it will stick to the same Opal build for anything besides a major release. Something like:

I think that's a good idea. We could also add a compatibility matrix table:

Asciidoctor reveal.js	Asciidoctor.js
3.x	2.x
4.x	3.x
5.x	3.x

But I would like to see that agreement of not breaking things somewhere in the Asciidoctor.js repo.

Asciidoctor.js now adheres to semantic versioning and binary-incompatibility is a breaking change.