-
Notifications
You must be signed in to change notification settings - Fork 188
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
Compiling slim templates into pure ruby (simplifies Javascript maintenance) #131
Compiling slim templates into pure ruby (simplifies Javascript maintenance) #131
Conversation
@obilodeau You can remove the following lines:
asciidoctor-reveal.js/package.json Line 43 in a1fddf4
Since @jirutka @obilodeau Is the |
templates/slim/helpers.rb
Outdated
@@ -3,14 +3,13 @@ | |||
require 'asciidoctor' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work in Asciidoctor.js because Opal will try to load this module from the OPAL_PATH
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s probably used only for the version check in this helpers.rb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that version check even necessary given that the way to use this module is through bundler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not necessary. I suggest to remove it.
templates/slim/helpers.rb
Outdated
@@ -3,14 +3,13 @@ | |||
require 'asciidoctor' | |||
require 'json' | |||
|
|||
# Needed only in compile-time. | |||
require 'slim-htag' if defined? Slim | |||
|
|||
if Gem::Version.new(Asciidoctor::VERSION) <= Gem::Version.new('1.5.3') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opal does not implement Gem
. You can still check the version in JavaScript with:
asciidoctor.getCoreVersion(); // available in the next release of Asciidoctor.js
Though you will need to use something like that to compare the version:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is not necessary, IMHO it should not be included in JS variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating my above comment: Is that version check even necessary given that the way to use this module is through bundler for ruby (which would take care of making sure proper version of asciidoctor is there)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it’s not necessary. I suggest to remove it.
I'm making progress on my branch https://github.com/mogztter/asciidoctor-reveal.js/tree/compile-slim-templates NOTE: You should use Mutable String methods are not supported in Opal:
In the generated converter, we are using _buf = ''
if (has_role? 'aside') or (has_role? 'speaker')
_buf << ("<aside class=\"notes\">".freeze)
_buf << ((content).to_s)
_buf << ("</aside>".freeze)
else
_buf << ("<div".freeze)
# ...
end Could we use the same strategy as Asciidoctor core and use an Array instead ? _buf = []
if (has_role? 'aside') or (has_role? 'speaker')
_buf << ("<aside class=\"notes\">".freeze)
_buf << ((content).to_s)
_buf << ("</aside>".freeze)
else
_buf << ("<div".freeze)
# ...
end
_buf * LF I'm not sure about the performance impact but if @mojavelinux is doing it in Asciidoctor core I think this should be fine ? @jirutka What do you think ? |
I've added a hack to replace mutable operations on String: ggrossetie@ef168e0#diff-32d7599354463e657c27d56ccc4b16bfR61 // Replace variable = '' by variable = []
data = data.replace(/([^ ]*) = ''/g, '$1 = \[\]');
// Replace _buf by _buf * ''
data = data.replace(/_buf$/g, '_buf * \'\'');
// Replace variable (alone in a single line) by variable * ''
data = data.replace(/^(\s*)(_[a-z1-9_\[\]]*)$/gm, '$1$2 = $2 * \'\''); For now I replace in-place the Here's the result:
|
I don’t know why it’s here, it doesn’t seem to be used in reveal.js templates.
Yes, just use different Temple generator – There’s currently no configuration option for this in asciidoctor-templates-compiler (I’ll add it), but you can subclass or patch Asciidoctor::TemplatesCompiler::Slim and modify
Hacks are not needed here (see above), Slim and especially Temple (backend for code generation) is nicely configurable. |
templates/slim/helpers.rb
Outdated
@@ -3,14 +3,13 @@ | |||
require 'asciidoctor' | |||
require 'json' | |||
|
|||
# Needed only in compile-time. | |||
require 'slim-htag' if defined? Slim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required, I’ve added it to my helpers.rb
just to allow using Slim templates even directly without ahead of time compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For development it might be useful to avoid the build step. Worth keeping as currently scoped in the unless opal
block. I'm very open to remove it if there's a good argument against though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said, it might be useful in development, for someone.
@@ -24,9 +25,10 @@ table(id=@id class=['tableblock',"frame-#{attr :frame, 'all'}","grid-#{attr :gri | |||
- cell_content = cell.text | |||
- else | |||
- cell_content = cell.content | |||
*{:tag=>(tblsec == :head ? 'th' : 'td'), :class=>['tableblock',"halign-#{cell.attr :halign}","valign-#{cell.attr :valign}"], | |||
= html_tag(tblsec == :head || cell.style == :header ? 'th' : 'td', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
html_tag
is not provided by Asciidoctor (or Slim), it’s defined in my helpers.rb
:
I was thinking about creating Slim/Temple extension like slim-htag, but for an arbitrary tag/element. Actually I’ve implemented it and it works, but I don’t know how to name it (and how to name the special attribute) and if it’s really a good thing to do: it’s not idiomatic and it does break The Principle of Least Surprise.
It may look like this:
??? [
??=(tblsec == :head || cell.style == :header ? 'th' : 'td')
class=['tableblock', "halign-#{cell.attr :halign}", "valign-#{cell.attr :valign}"]
colspan=cell.colspan
rowspan=cell.rowspan
style=((@document.attr? :cellbgcolor) ? %(background-color:#{@document.attr :cellbgcolor};) : nil) ]
???
should be name of the special “tag” (e.g. element
, dyntag
, …?) and ??
name of the special attribute defining the actual tag (e.g. tag
, _
, …?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After quickly looking at HTML documentation I would say ???
be html_element
and ??
= tag_name
but that's far from compact. dyntag
and tag
would work for me as shorter alternatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime, I've added your html_tag
function to our helpers.rb
Super nice 🤓
I didn't know about
Ok I will remove my hack and give it a try. |
@jirutka It's almost working, the buffer is now an array but there's still some String variables. Do you think we can configure how these variables are generated ? _buf = []
_buf << ("<div".freeze) # OK _temple_html_attributeremover1 = ''
_temple_html_attributeremover1 << ((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s) # KO |
@obilodeau As mentioned by @jirutka, we should remove the generated file |
It seems that you have to set |
@Mogztter I pulled in your changes and added some of mine on top. Ideally to avoid rebasing it would be nice if you would pull my tree (or start a branch from scratch) so that we keep the history clean. |
Indeed, we can configure It's one step closer but the generated code still use before.rb _buf = []
_buf << ("<div".freeze)
_temple_html_attributeremover1 = ''
_temple_html_attributemerger1 = []
_temple_html_attributemerger1[0] = "paragraph"
_temple_html_attributemerger1[1] = ''
# ... KO!
_temple_html_attributeremover1 << ((_temple_html_attributemerger1.reject(&:empty?).join(" ")).to_s)
_temple_html_attributeremover1
# ... KO!
_temple_html_attributemerger1[1] << ((_slim_codeattributes1).to_s) after.rb _buf = []
_buf << ("<div".freeze)
_temple_html_attributeremover1 = []
_temple_html_attributemerger1 = []
_temple_html_attributemerger1[0] = "paragraph"
_temple_html_attributemerger1[1] = ''
# ... OK
temple_html_attributeremover1 << (_temple_html_attributemerger1.reject(&:empty?).join(" "))
_temple_html_attributeremover1 = _temple_html_attributeremover1.join("".freeze)
# ... KO!
_temple_html_attributemerger1[1] << ((_slim_codeattributes1).to_s) |
@obilodeau I've updated my branch: https://github.com/Mogztter/asciidoctor-reveal.js/commits/compile-slim-templates EDIT: I've added a |
@Mogztter: your changes have been pushed. |
Thanks! Also the build does not work on Ruby 2.1 and Ruby 2.2 since the library |
Also the build with
|
Hm, this looks like a bug in Slim or Temple. I’ll take a look at it later.
Yeah, because I use
That’s bad, ruby-beautify executes You can avoid this error by running only fast compilation ( EDIT: I’ve patched it in |
templates/slim/helpers.rb
Outdated
## | ||
# This function is from the asciidictor-html5s project | ||
# Copyright 2014-2017 Jakub Jirutka <jakub@jirutka.cz> and the Asciidoctor Project. | ||
# Licensed under the MIT License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not needed to copy copyright here; both projects are MIT licensed and contain Asciidoctor project as one of the copyright holders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
templates/slim/helpers.rb
Outdated
attrs_str = attrs.empty? ? '' : attrs.join(' ').prepend(' ') | ||
|
||
|
||
if VOID_ELEMENTS.include? name.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where’s VOID_ELEMENTS
constant…?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left behind. I added it.
It’s a bug in Temple, I’ve fixed it and opened PR judofyr/temple#112.
I’ve fixed it, opened PR erniebrodeur/ruby-beautify#43 and patched it in It has also nice side effect – significantly faster code beautification. Thus we don’t need
I’ve opened PR judofyr/temple#113 to change default |
Awesome! Thanks @jirutka 💯 |
TODO
|
Merged with master (which now has doctest testing #116). Addressed several comments. Doctest are failing, I think it is all related to the handling of I'll try to fix doctest failures. It's just too late now to continue working on this.
I could use a hand on this. |
This means that slim-htag was not loaded. |
doctests were not using the ruby back-end but the slim templates directly. Fixed it. |
Hm, still it should work; templates/slim/helpers.rb:7 does not do the job for some reason. :/ EDIT: It really does not work, because Asciidoctor loads helpers after initializing Slim. So one must take care of loading |
FYI, if you want to fallback to built-in converter for nodes you don’t have template for, set |
I’ve released asciidoctor-templates-compiler 0.2.0 with these important additions:
So you can remove all the workarounds. I found out that Opal does not handle |
Rakefile
Outdated
@@ -28,6 +33,10 @@ namespace :build do | |||
outfilesuffix: '.html', | |||
filetype: 'html', | |||
}, | |||
delegate_backend: 'html5', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation… 😠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. My vim isn't properly setup for ruby still 😞
templates/slim/helpers.rb
Outdated
@@ -5,10 +5,6 @@ | |||
|
|||
# Needed only in compile-time. | |||
require 'slim-htag' if defined? Slim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this as well, it doesn’t work as intended (for direct use of templates) anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Current status:
build.js: // Load asciidoctor.js + asciidoctor-reveal.js
var asciidoctor = require('asciidoctor.js')();
var Asciidoctor = asciidoctor.Asciidoctor();
require('asciidoctor-reveal.js');
// Convert the document 'presentation.adoc' using Reveal.js backend
var attributes = 'revealjsdir=node_modules/reveal.js@';
var options = {safe: 'safe', backend: 'revealjs', attributes: attributes};
Asciidoctor.convertFile('presentation.adoc', options); package.json: {
"name": "presentation-asciidoctorjs-test",
"version": "1.0.0",
"description": "",
"main": "build.js",
"dependencies": {
"asciidoctor-reveal.js": "file:../asciidoctor-reveal.js",
"asciidoctor.js": "^1.5.5-5"
},
"devDependencies": {},
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC"
} package-lock.json: https://gist.github.com/obilodeau/bfba282a43e5e9572ce1d4f7cdf06b69 @Mogztter any hints? |
You need to remove this line : var Asciidoctor = asciidoctor.Asciidoctor(); And use asciidoctor.convertFile('presentation.adoc', options); Also attributes can now be defined as JSON object: var attributes = { 'revealjsdir': 'node_modules/reveal.js@' }; EDIT: ggrossetie@3e9ede2 |
@obilodeau We also need to update |
@Mogztter I pulled in your change and updated As expected, I'm stuck at the
I'm going to add a task to wait until the fix is released before rebase/review time. |
@jirutka I tried this. Using two different approaches. I'm close but it still doesn't work... Can you look at it and give me a pointer? Attempt 1Build matrix with shell environment variable and I alter my script's behavior. Remaining problems: on 2.2it fails because it says it requires temple (althought it shouldn't) but the on 2.1
Attempt 2Forcing ruby 2.4 on all Problems: 2.4, 2.3, jrubyrake missing: https://travis-ci.org/obilodeau/asciidoctor-reveal.js/jobs/272211812, jruby: https://travis-ci.org/obilodeau/asciidoctor-reveal.js/jobs/272211820 2.2
https://travis-ci.org/obilodeau/asciidoctor-reveal.js/jobs/272211816 2.1
https://travis-ci.org/obilodeau/asciidoctor-reveal.js/jobs/272211818 I've spent way to much time on this than I'm willing to admit. Guidance would be appreciated. |
There are more traps than I’ve expected, but it works:
I’ll reconsider adding support for older rubies to asciidoctor-templates-compiler. |
Okay, I’ve just released asciidoctor-templates-compiler 0.3.0 that is compatible with Ruby 2.1+ (and also adds CLI). |
4ee53dd
to
ea3e127
Compare
Tonight:
Will file these issues:
Ready for review. I think this work can go into master and get more exposure now. |
Took the opportunity to drop the `block_` prefix in template names to align with other asciidoctor plugins
57157c9
to
b7f1c25
Compare
npm/builder.js
Outdated
} | ||
}; | ||
|
||
Builder.prototype.replaceUnsupportedFeatures = function (callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to state that this is hack that need to be removed once issue XX is resolved ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already not needed, see my comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just removed it and confirmed things reach the same ThreadSafe::Cache
error without it. It was reintroduced in the rebase process. The rebase was quite complicated to merge since that branch started before doctest was in and master was pulled in at some point.
I'm leveraging asciidoctor-templates-compiler by @jirutka. The goal here is to get rid of the jade templates and only have to maintain one set of templates. See #63.
This is not ready, I want doctest (#116) merged in first to see for potential regressions using doctest.Will rebase/fix once it is in.
re-enable ruby 2.1, 2.2 travis support by compiling templates using a recent version of ruby but using it from an old oneWaiting on upstream
replaceUnsupportedFeatures
function from npm/builder.js once [Bugfix] Use the specified capture_generator even for nested captures judofyr/temple#112 is merged and releasedcapture_generator
from lib/asciidoctor-templates-compiler.rb once Change default :capture_generator to self judofyr/temple#113 is merged and released (or at least fix the typocapture_generate
->capture_generator
)Thanks to @jirutka for the asciidoctor-templates-compiler! It took me only an hour maybe to get the PoC running.