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

Upgrade to Asciidoctor.js 2.0.0 #252

Closed
ggrossetie opened this issue Mar 31, 2019 · 10 comments · Fixed by #254
Closed

Upgrade to Asciidoctor.js 2.0.0 #252

ggrossetie opened this issue Mar 31, 2019 · 10 comments · Fixed by #254

Comments

@ggrossetie
Copy link
Member

ggrossetie commented Mar 31, 2019

The final version is not yet available but I've published a second release candidate today.
A few notes:

  • The package was renamed to asciidoctor
  • The package asciidoctor is now a meta-package that contains @asciidoctor/core and @asciidoctor/cli
  • We probably should rename the package to @asciidoctor/reveal.js-converter
  • We should make sure that the following is working:
$ npm i asciidoctor @asciidoctor/reveal.js-converter
$ asciidoctor -r @asciidoctor/reveal.js-converter -b reveal.js presentation.adoc
@mojavelinux
Copy link
Member

@asciidoctor/converter-reveal.js

When I come back to this with fresh eyes, the order strikes me as awkward. As much as the programmer in me wants it to be inverted, I think we should use @asciidoctor/reveal.js-converter instead.

@ggrossetie
Copy link
Member Author

If we want the following to be working, we will need to agree on an API.

$ asciidoctor -r @asciidoctor/reveal.js-converter -b reveal.js presentation.adoc

The reason is that the CLI will have to "automatically" register the converter.
We should also consider that we could register extensions:

$ asciidoctor -r @asciidoctor/reveal.js-converter -r @asciidoctor/plantuml-extension -b reveal.js presentation.adoc

In my opinion it's more intuitive than to require the user to create a JavaScript file to load and register the converters/extensions:

init.js

require('@asciidoctor/reveal.js-converter').register()
$ asciidoctor -r ./init.js -b reveal.js presentation.adoc

@obilodeau
Copy link
Member

@asciidoctor/converter-reveal.js

When I come back to this with fresh eyes, the order strikes me as awkward. As much as the programmer in me wants it to be inverted, I think we should use @asciidoctor/reveal.js-converter instead.

@Mogztter's PR I just merged didn't contain the package name change. Since this release is going to be a major one due to the Node API change, I suggest we change the package name right now if it still makes sense to do so.

Should this be renamed @asciidoctor/reveal.js-converter or did our mind change about that?

To me @asciidoctor/reveal.js would be even better since it is unlikely that we will ever have a non-converter reveal.js asciidoctor extension. I would be open to @asciidoctor/converter/reveal.js too. @mojavelinux's suggestion (@asciidoctor/reveal.js-converter) is also good btw, I was just trying to make it shorter.

@ggrossetie
Copy link
Member Author

I just merged didn't contain the package name change. Since this release is going to be a major one due to the Node API change, I suggest we change the package name right now if it still makes sense to do so.

You're right.
I reverted my changes because I wasn't sure it will be included in a major release.

Should this be renamed @asciidoctor/reveal.js-converter or did our mind change about that?
To me @asciidoctor/reveal.js would be even better since it is unlikely that we will ever have a non-converter reveal.js asciidoctor extension.

I had the same thought and I think we can safely remove the extra -converter when the name is an output format (pdf, reveal.js, epub...).
In my opinion, @asciidoctor/reveal.js is concise, non-ambiguous and it reads well.

I will wait for @mojavelinux's approval to make sure that we are on the same page.

@obilodeau
Copy link
Member

@mojavelinux: @Mogztter and I would go with @asciidoctor/reveal.js as the new module name for the javascript ecosystem for reasons stated above. The PR is ready (see #291). This is a breaking change and it will happen in what will become 3.0.0. We would like to move forward. Do we get your approval?

@mojavelinux
Copy link
Member

I had the same thought and I think we can safely remove the extra -converter when the name is an output format (pdf, reveal.js, epub...).

I think that's a reasonable and intutive assumption. I'll vote 👍 on @asciidoctor/reveal.js.

@obilodeau
Copy link
Member

With #291 merged, now we probably need to do something on npm. Releasing an interim version of @asciidoctor/reveal.js like 3.0.0rc1 would probably make sense to reserve the spot and make instructions on the README work. Smoke tests probably need changes too.

@ggrossetie
Copy link
Member Author

With #291 merged, now we probably need to do something on npm. Releasing an interim version of @asciidoctor/reveal.js like 3.0.0rc1 would probably make sense to reserve the spot and make instructions on the README work. Smoke tests probably need changes too.

I think it's a good idea 👍

@obilodeau
Copy link
Member

Release candidate 1 packages are out there: https://www.npmjs.com/package/@asciidoctor/reveal.js

@obilodeau
Copy link
Member

I just tested the javascript API (which resulted in this PR #298 to update our Babel example API usage) and it works.

I also tested the README setup instructions:

npm i --save asciidoctor @asciidoctor/reveal.js
$(npm bin)/asciidoctor --version
$(npm bin)/asciidoctor -r @asciidoctor/reveal.js -b reveal.js README.adoc

And it worked.

This successfully completes all remaining tasks in this ticket. Closing.

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

Successfully merging a pull request may close this issue.

3 participants