resolves #143, Upgrade to Asciidoctor.js 1.5.6-preview.4 #156

ggrossetie · 2017-10-08T13:17:59Z

Create a Rake task to generate a version of the Ruby converter which is compatible with Opal
Add asciidoctor.js as a dependency
Update HACKING and README accordingly

Resolves #143
Ref #130

* Create a Rake task to generate a version of the Ruby converter which is compatible with Opal * Add asciidoctor.js as a dependency * Update HACKING and README accordingly

obilodeau · 2017-10-10T21:04:29Z

Why not pull the test I developed in #149 instead?

ggrossetie · 2017-10-10T21:27:22Z

I don't think tests should be part of the builder script but I like the idea of having "examples".
We could reuse your task to demonstrate how to use the Reveal.js converter in JavaScript ?

obilodeau · 2017-10-10T21:40:04Z

I don't think tests should be part of the builder script but I like the idea of having "examples".We could reuse your task to demonstrate how to use the Reveal.js converter in JavaScript ?

Sure but first, I'm try to make sure I can have a working asciidoctor-reveal.js from javascript.

obilodeau · 2017-10-10T21:43:13Z

Pulled in your branch on current master then ran:

bundle exec rake build:converter:opal
npm install                                                 # for the changed deps
npm run build
npm run test

It crashes with:

$ npm run test

> asciidoctor-reveal.js@1.1.0-dev test /home/olivier/src/asciidoc/asciidoctor-reveal.js
> node test/smoke.js


/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:4993
      throw exception;
      ^
NotImplementedError: String#<< not supported. Mutable String methods are not supported in Opal.
    at String.TMP_String_$lt$lt_1 [as $<<] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:20858:19)
    at $Document.TMP_55 (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:1933:20)
    at $Document.$$instance_eval [as $instance_eval] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:3619:24)
    at Opal.send (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1605:19)
    at $Converter.$$document [as $document] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:1922:14)
    at $Converter.$$__send__ [as $__send__] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:3546:21)
    at Object.Opal.send (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1605:19)
    at $Converter.alias [as $send] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1779:19)
    at $Converter.$$convert [as $convert] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:211:26)
    at $Document.$$convert [as $convert] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/asciidoctor.js/dist/asciidoctor.js:27996:35)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! asciidoctor-reveal.js@1.1.0-dev test: `node test/smoke.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the asciidoctor-reveal.js@1.1.0-dev test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/olivier/.npm/_logs/2017-10-10T21_33_31_209Z-debug.log

I'm trying from a clean directory using npm i ../asciidoctor-reveal.js and i'll report back. Still I expected smoke testing to work from inside the repo.

ggrossetie · 2017-10-10T21:52:26Z

The converter task does not replace the generated file if it exists. Run the clean task first and you should be fine. Le 10 oct. 2017 11:43 PM, "Olivier Bilodeau" <notifications@github.com> a écrit :

…

Pulled in your branch on current master then ran: bundle exec rake build:converter:opal npm install # for the changed deps npm run build npm run test It crashes with: $ npm run test > ***@***.*** test /home/olivier/src/asciidoc/asciidoctor-reveal.js > node test/smoke.js /home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:4993 throw exception; ^ NotImplementedError: String#<< not supported. Mutable String methods are not supported in Opal. at String.TMP_String_$lt$lt_1 [as $<<] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:20858:19) at $Document.TMP_55 (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:1933:20) at $Document.$$instance_eval [as $instance_eval] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:3619:24) at Opal.send (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1605:19) at $Converter.$$document [as $document] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:1922:14) at $Converter.$$__send__ [as $__send__] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:3546:21) at Object.Opal.send (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1605:19) at $Converter.alias [as $send] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/opal-runtime/src/opal.js:1779:19) at $Converter.$$convert [as $convert] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/dist/main.js:211:26) at $Document.$$convert [as $convert] (/home/olivier/src/asciidoc/asciidoctor-reveal.js/node_modules/asciidoctor.js/dist/asciidoctor.js:27996:35) npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! ***@***.*** test: `node test/smoke.js` npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the ***@***.*** test script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above. npm ERR! A complete log of this run can be found in: npm ERR! /home/olivier/.npm/_logs/2017-10-10T21_33_31_209Z-debug.log I'm trying from a clean directory using npm i ../asciidoctor-reveal.js and i'll report back. Still I expected smoke testing to work from inside the repo. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#156 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUV3OsOnKAcy1UGqnFmEBxOVHtoq6k2ks5sq-TygaJpZM4Pxs4W> .

obilodeau · 2017-10-10T22:03:03Z

HACKING.adoc

@@ -261,6 +261,10 @@ Then run:
 $ gem push asciidoctor-revealjs-X.Y.Z.gem

 . Check that the new version is available on https://rubygems.org/gems/asciidoctor-revealjs[rubygems.org]
+. Generate a compatible version of the Ruby converter (using opal mode)
+
+ $ bundle exec rake build:converter:opal


A rake clean is missing here but I think it should be done automatically whenever we build a template converter. Can you think of a reason why we shouldn't?

I agree, the build task should always generate the file (even if it already exists) since we can generate the file using different modes.

obilodeau · 2017-10-10T22:06:19Z

Running bundle exec rake clean then rebuilding fixed it! It worked also using HACKING.adoc's documentation to test a local npm package before release.

obilodeau · 2017-10-10T22:07:30Z

https://github.com/asciidoctor/asciidoctor-reveal.js/pull/156/files#diff-18bed20d24ebdfacf77001ffe1d9617bR216

This should be bumped to preview.4

ggrossetie · 2017-10-11T06:23:28Z

This should be bumped to preview.4

The link does not point to a single line. Where do you want to bump the version ?

ggrossetie · 2017-10-11T17:22:21Z

As mentioned in #143 (comment) I will update the Rake task to follow @jirutka recommandation

obilodeau · 2017-10-11T20:38:27Z

This should be bumped to preview.4

The link does not point to a single line. Where do you want to bump the version ?

Sorry, I wasn't able to comment since it's not in the patch's context.

HACKING.adoc line 216:

  $ npm i --save asciidoctor.js@1.5.6-preview.3

I would have done it but I had time to wait for your feedback on what you think of @jirutka's comments in #143 regarding the Rakefile and these I wanted to leave to you to decide upon

ggrossetie · 2017-10-11T21:17:13Z

@obilodeau Nice catch I totally missed it! 🤓
Actually it's not needed anymore since Asciidoctor.js is now a direct dependency of the Reveal.js converter. In other words, npm install asciidoctor-reveal.js will also install asciidoctor.js, so we can drop this line.

Also I think we should remove the section about Jade templates and help AsciidocFX migrate to the new version of the Reveal.js converter. Ping @rahmanusta 😉

obilodeau · 2017-10-11T23:44:03Z

Actually it's not needed anymore since Asciidoctor.js is now a direct dependency of the Reveal.js converter. In other words, npm install asciidoctor-reveal.js will also install asciidoctor.js, so we can drop this line.

This section is about doing pre-release testing. It was written when I was facing problems trying to reproduce an environment required to render slides before pushing the version to npm: https://github.com/asciidoctor/asciidoctor-reveal.js/blob/master/HACKING.adoc#test-a-local-asciidoctor-reveal-js-version.

Issue #148 tries to explain the problem:

The problem is when installing using npm and ../asciidoctor-reveal.js (a relative dir instead of a package), a symbolic link is installed under node_modules/ and the package's dependencies are not installed locally (flat). I guess their rationale is that the package under development already has all its dependencies in its own node_modules/. It's not clear so I'll do an example:

With the npm i --save asciidoctor.js@1.5.6-preview.4 step:

olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ !5004
npm i --save asciidoctor.js@1.5.6-preview.4
npm WARN saveError ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm WARN presentation-asciidoctorjs-test No description
npm WARN presentation-asciidoctorjs-test No repository field.
npm WARN presentation-asciidoctorjs-test No README data
npm WARN presentation-asciidoctorjs-test No license field.

+ asciidoctor.js@1.5.6-preview.4
added 12 packages in 1.769s
olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ npm i --save ../asciidoctor-reveal.js
npm WARN saveError ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm WARN enoent ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm WARN presentation-asciidoctorjs-test No description
npm WARN presentation-asciidoctorjs-test No repository field.
npm WARN presentation-asciidoctorjs-test No README data
npm WARN presentation-asciidoctorjs-test No license field.

+ asciidoctor-reveal.js@1.1.0-dev
added 1 package in 0.66s


olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ ls -l node_modules/
total 48
drwxr-xr-x 3 olivier olivier 4096 Oct 11 19:37 asciidoctor.js
lrwxrwxrwx 1 olivier olivier   27 Oct 11 19:37 asciidoctor-reveal.js -> ../../asciidoctor-reveal.js
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 balanced-match
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 brace-expansion
drwxr-xr-x 4 olivier olivier 4096 Oct 11 19:37 concat-map
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 glob
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 inflight
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 inherits
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 minimatch
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 once
drwxr-xr-x 5 olivier olivier 4096 Oct 11 19:37 opal-runtime
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 path-is-absolute
drwxr-xr-x 2 olivier olivier 4096 Oct 11 19:37 wrappy

Without it:

olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ npm i --save ../asciidoctor-reveal.js
npm WARN saveError ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open '/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/package.json'
npm WARN presentation-asciidoctorjs-test No description
npm WARN presentation-asciidoctorjs-test No repository field.
npm WARN presentation-asciidoctorjs-test No README data
npm WARN presentation-asciidoctorjs-test No license field.

+ asciidoctor-reveal.js@1.1.0-dev
added 1 package in 0.532s


olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ ls -l node_modules/
total 0
lrwxrwxrwx 1 olivier olivier 27 Oct 11 19:39 asciidoctor-reveal.js -> ../../asciidoctor-reveal.js

olivier@lafindumonde:~/src/asciidoc/presentation-asciidoctorjs-test$ node asciidoctor-revealjs.js 
module.js:529
    throw err;
    ^

Error: Cannot find module 'asciidoctor.js'
    at Function.Module._resolveFilename (module.js:527:15)
    at Function.Module._load (module.js:476:23)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/olivier/src/asciidoc/presentation-asciidoctorjs-test/asciidoctor-revealjs.js:2:19)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)

So for the README (or normal user) use case: no npm i asciidoctor.js required but for pre-release testing of the results of the build w/o pushing to npm, an npm i asciidoctor.js is required, thus leave the doc in the HACKING guide..

ggrossetie · 2017-10-12T06:07:16Z

Interesting, I didn't know that 😐
Does it change anything if you do a npm init -y before running npm i --save ../asciidoctor-reveal.js ? It should at least remove the warning ?
What version of npm/node are you using ? I wonder if all npm versions behave the same... And also what is the behavior of yarn... 🤔

Thank you for pointing that out, I will do some testing this morning 🐛 🔨

ggrossetie · 2017-10-12T07:42:39Z

The behavior seems to have changed in npm 5+

npm install ./packages/subdir will now create a symlink instead of a regular installation. file://path/to/tarball.tgz will not change -- only directories are symlinked. (#15900)

https://github.com/npm/npm/releases/tag/v5.0.0

It's weird because it's a pretty big breaking change and they do not provide a clear alternative... or am I missing something ?

For reference yarn@1.1.0 is doing a regular installation.

As a workaround, we could use npm pack inside asciidoctor-reveal.js directory and then npm install ../asciidoctor-reveal.js/asciidoctor-reveal.js-1.1.0-dev.tgz.
We can also keep the explicit installation of asciidoctor.js but I'm not a huge fan because it's not DRY (we are repeating what we described in package.json).
Having said that, I think the safest choice for now is just to update the version in HACKING.adoc. What do you think @obilodeau ?

obilodeau · 2017-10-12T13:18:28Z

npm init -y same behavior. FYI: We are discussing adding that step in here #141 (comment) but I'm against it so far since it's not required (at least on my setup).

I'm open to switching to yarn but in another PR / release.

npm pack works like a charm! It has the advantage of mimicking the way I test releases on the ruby side of things. In addition, it does exactly what would happen if you would install the package from the npm repo. I advise we scrap the asciidoctor.js install and replace it with npm pack and npm i --save ../asciidoctor-reveal.js/asciidoctor-reveal.js-<pkgversion>.tgz. Great find!

obilodeau · 2017-10-12T23:29:08Z

Thanks again for your help through this!

ggrossetie added 2 commits October 8, 2017 15:16

resolves asciidoctor#143, Upgrade to Asciidoctor.js 1.5.6-preview.4

2049624

* Create a Rake task to generate a version of the Ruby converter which is compatible with Opal * Add asciidoctor.js as a dependency * Update HACKING and README accordingly

Adds smoke test

f94965c

ggrossetie mentioned this pull request Oct 10, 2017

Converting the examples using javascript / node toolchain #149

Merged

obilodeau reviewed Oct 10, 2017

View reviewed changes

obilodeau added this to the 1.1.0 milestone Oct 11, 2017

obilodeau added 🍩 improvement 📜 javascript/node labels Oct 11, 2017

ggrossetie added 2 commits October 12, 2017 19:11

Update HACKING instruction on how to test the Node package

6990dd6

Simplify Rakefile and use clean task before generating converter.rb

24be4cb

obilodeau merged commit 1f237af into asciidoctor:master Oct 12, 2017

obilodeau mentioned this pull request Oct 12, 2017

Upgrade to Asciidoctor.js 1.5.6+ #130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolves #143, Upgrade to Asciidoctor.js 1.5.6-preview.4 #156

resolves #143, Upgrade to Asciidoctor.js 1.5.6-preview.4 #156

ggrossetie commented Oct 8, 2017 •

edited

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 10, 2017

obilodeau commented Oct 10, 2017

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 10, 2017 via email

obilodeau Oct 10, 2017

ggrossetie Oct 11, 2017

obilodeau commented Oct 10, 2017

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 11, 2017

ggrossetie commented Oct 11, 2017

obilodeau commented Oct 11, 2017

ggrossetie commented Oct 11, 2017

obilodeau commented Oct 11, 2017

ggrossetie commented Oct 12, 2017

ggrossetie commented Oct 12, 2017

obilodeau commented Oct 12, 2017

obilodeau commented Oct 12, 2017

resolves #143, Upgrade to Asciidoctor.js 1.5.6-preview.4 #156

resolves #143, Upgrade to Asciidoctor.js 1.5.6-preview.4 #156

Conversation

ggrossetie commented Oct 8, 2017 • edited

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 10, 2017

obilodeau commented Oct 10, 2017

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 10, 2017 via email

obilodeau Oct 10, 2017

Choose a reason for hiding this comment

ggrossetie Oct 11, 2017

Choose a reason for hiding this comment

obilodeau commented Oct 10, 2017

obilodeau commented Oct 10, 2017

ggrossetie commented Oct 11, 2017

ggrossetie commented Oct 11, 2017

obilodeau commented Oct 11, 2017

ggrossetie commented Oct 11, 2017

obilodeau commented Oct 11, 2017

ggrossetie commented Oct 12, 2017

ggrossetie commented Oct 12, 2017

obilodeau commented Oct 12, 2017

obilodeau commented Oct 12, 2017

ggrossetie commented Oct 8, 2017 •

edited