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

By default the revealjsdir attribute, in a JavaScript and node environment, should be 'node_modules/reveal.js@' #191

Closed
ggrossetie opened this issue Apr 22, 2018 · 11 comments

Comments

@ggrossetie
Copy link
Member

It will make the installation/configuration easier.

@obilodeau
Copy link
Member

I see your point. This will complicate doctests though but it's ok, they don't work yet. Oh and I'll fix them.

Made some rough tests and something like this in slim seems to work:

    - if RUBY_ENGINE == 'opal'
      - revealjsdir = (attr :revealjsdir, 'node_modules/reveal.js@')
    - else
      - revealjsdir = (attr :revealjsdir, 'reveal.js')
  • Would that be the proper approach?
  • Is the @ necessary? What does it do?

@mojavelinux
Copy link
Member

Would that be the proper approach?

Opal only tells you that it's JavaScript, not necessarily that it's Node. But you can check for a Node environment either by the presence of this folder or Asciidoctor.js can tell you what environment it has loaded.

Is the @ necessary? What does it do?

The @ allows the document to override the attribute. See https://asciidoctor.org/docs/user-manual/#altering-the-attribute-assignment-precedence

@obilodeau
Copy link
Member

Would that be the proper approach?

Opal only tells you that it's JavaScript, not necessarily that it's Node. But you can check for a Node environment either by the presence of this folder or Asciidoctor.js can tell you what environment it has loaded.

But given current Javascript usage patterns, testing for Opal should suffice, no? I don't necessarily want to write a rather move involved test looking at the filesystem or trying to poke Asciidoctor.js's environment but having to add boilerplate code not to crash when called from Ruby.

Is the @ necessary? What does it do?

The @ allows the document to override the attribute. See https://asciidoctor.org/docs/user-manual/#altering-the-attribute-assignment-precedence

Ah I forgot about that. But isn't the use of attr attribute, default, already gives precedence to a document-defined attribute? I mean, I don't see a problem specifying @ and using attr but I wanted to make sure I understood what it meant. Thanks.

@mojavelinux
Copy link
Member

But given current Javascript usage patterns, testing for Opal should suffice, no?

I think that would be closing doors on running the converter in the browser, as @Mogztter often points out.

trying to poke Asciidoctor.js's environment

Asciidoctor.js already does the hard work, so it's best to leverage the API it is providing. Specifically, you'd be looking at http://asciidoctor.github.io/asciidoctor.js/master/#asciidoctorgetruntime

But isn't the use of attr attribute, default, already gives precedence to a document-defined attribute?

No, that's just a shorthand for getting the attribute from the map.

I mean, I don't see a problem specifying @ and using attr but I wanted to make sure I understood what it meant.

Oops, my mistake. The @ doesn't apply in this context (and thus shouldn't be used). It's only used when invoking the converter (either via the API or the CLI). So ignore what I said before.

@obilodeau
Copy link
Member

But given current Javascript usage patterns, testing for Opal should suffice, no?

I think that would be closing doors on running the converter in the browser, as @Mogztter often points out.

Since we compile the slim templates to ruby then to javascript with Opal and releasing the result as an npm package, anything consuming that package would have the node_modules/reveal.js path. To me that closes the gap for any javascript-based usage but I think I have a blind spot here.

How is that javascript going to be distributed to the browser otherwise?

@obilodeau
Copy link
Member

Asciidoctor.js already does the hard work, so it's best to leverage the API it is providing. Specifically, you'd be looking at http://asciidoctor.github.io/asciidoctor.js/master/#asciidoctorgetruntime

I looked at the API and I don't see how we will be able to call it from the slim templates... Googling for "Opal inline javascript" didn't give what I expected. Guidance required.

@mojavelinux
Copy link
Member

How is that javascript going to be distributed to the browser otherwise?

Using browserify / webpack (and then probably bundled with a program, such as with AsciidocFX). But then it would be incorrect to look in node_modules to find reveal.js. Of course, that doesn't rule out that integration simply overridding the value.

Still, we shouldn't assume anywhere that RUBY_ENGINE == 'opal' is Node as a general practice.

@mojavelinux
Copy link
Member

Inside the RUBY_ENGINE == 'opal' block, you'd need something like this:

if JAVASCRIPT_PLATFORM == 'node'
  ...
end

keep in mind this constant will not be defined in Ruby, so it has to be inside the if RUBY_ENGINE == 'opal' block.

Here's where all this gets defined: https://github.com/asciidoctor/asciidoctor.js/blob/master/lib/asciidoctor/js/opal_ext/node.rb

@mojavelinux
Copy link
Member

I defer to @Mogztter to confirm.

@obilodeau obilodeau changed the title By default the revealjsdir attribute, in a JavaScript environment, should be 'node_modules/reveal.js@' By default the revealjsdir attribute, in a JavaScript and node environment, should be 'node_modules/reveal.js@' Nov 27, 2018
@obilodeau
Copy link
Member

Discussed with @Mogztter last week on twitter. PR is coming.

@obilodeau
Copy link
Member

PR is #228

obilodeau added a commit that referenced this issue Nov 28, 2018
Added a different default value for revealjsdir under node (fixes #191)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants