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

Load converter when used. #11

Merged
merged 2 commits into from Oct 11, 2015
Merged

Load converter when used. #11

merged 2 commits into from Oct 11, 2015

Conversation

wh0
Copy link

@wh0 wh0 commented Oct 11, 2015

cross reference: jekyll/jekyll#2327

Make it so that you can use Jekyll 2.x without a JavaScript runtime, as long as you don't need to compile CoffeeScript.

Sample error when you try to do that:

$ jekyll b
Configuration file: /home/guest1212/Workspaces/site/_config.yml
            Source: /home/guest1212/Workspaces/site
       Destination: /home/guest1212/Workspaces/site/_site
      Generating... 
  Conversion error: Jekyll::Converters::CoffeeScript encountered an error while converting 'test.coffee':
                    Could not find a JavaScript runtime. See https://github.com/rails/execjs for a list of available runtimes.
jekyll 2.5.3 | Error:  Could not find a JavaScript runtime. See https://github.com/rails/execjs for a list of available runtimes.

@parkr
Copy link
Member

parkr commented Oct 11, 2015

Great! In the Markdown Converter in Jekyll core, this is gated via an instance variable. I think requiring each time could be dangerous performance wise, so doing something like this would be preferable:

def setup
  require "coffee-script"
  @setup = true
end

def convert(content)
  setup unless @setup
  ::CoffeeScript...
end

@wh0
Copy link
Author

wh0 commented Oct 11, 2015

Thanks, I've updated it to be like https://github.com/jekyll/jekyll/blob/master/lib/jekyll/converters/markdown.rb.

ℹ️ http://ruby-doc.org/core-2.2.3/Kernel.html#method-i-require
Kernel::require has its own mechanism for preventing multiple loads of the same module though.

@parkr
Copy link
Member

parkr commented Oct 11, 2015

I'm trying to reduce method calls. Did you not like my example above?

@wh0
Copy link
Author

wh0 commented Oct 11, 2015

force pushed.

@parkr
Copy link
Member

parkr commented Oct 11, 2015

Ok, just asking. Thank you! This looks good to me. :shipit:

parkr added a commit that referenced this pull request Oct 11, 2015
Load converter when used.
@parkr parkr merged commit 27b8c7d into jekyll:master Oct 11, 2015
parkr added a commit that referenced this pull request Oct 11, 2015
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants