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

"capturing" outside of :erb #1066

Closed
sunshineco opened this Issue Jan 15, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@sunshineco
Contributor

sunshineco commented Jan 15, 2017

On my websites, I import several stand-alone HTML files verbatim into content as first-class nanoc items which should be incorporated into the website as proper pages, thus laid out and styled the same as the rest of the pages.

For example, a source code project's README.html may be imported into the website as nanoc item /README.html which is routed to /documentation/index.html. Since these are full-blown HTML files, the enclosing <html> and <body> elements need to be stripped and the body content wrapped instead inside in a <div> to become the item's compiled_content which is then laid-out via a normal layout invocation in a compile rule. This stripping of the unwanted HTML elements should be automated (I shouldn't have to do it by hand when importing the HTML file), thus seems a natural fit for a filter. For instance:

compile '/README.*' do
    filter :imported_html
    layout '/default.*'
    write '/documentation/index.html'
end

In some cases, these foreign HTML files also have <style> and <script> elements in <head> which need to be incorporated into <head> of the item laid out by nanoc. For example, in layouts/default.html:

<html>
    <head>
        ...
        <%= content_for(@item, :head) %>

The imported_html filter looks something like this:

class ImportedHtml < Nanoc::Filter
  identifiers :imported_html

  def run(content, params={})
    content_for(:head) do
      # Grab <style> and <script> elements from <head>.
      ...
      styles + scripts
    end

    # Grab <body> content.
    ...
    '<div>' + body + '</div>'
  end
end

The extracted <style> and <script> elements need to be stored somewhere so that they can later be accessed by the layout; since filtering is happening at compile-time, they can't be stored as item attributes (which are frozen), thus storing them as content_for seems appropriate.

Unfortunately, this doesn't work. content_for expects to be called within the context of the :erb filter and fails when called outside (say, from some other filter, such as imported_html). Specifically, content_for fails since there is no _erbout. As a work-around, rather than calling content_for directly, the filter instead calls capture_for which is defined locally as:

def capture_for(tag)
  _erbout = ''
  content_for(tag) { _erbout << yield }
end

Although this works, it is ugly since it breaks the abstraction of nanoc's capturing helper by having special knowledge of its implementation.

A cleaner solution would be for nanoc's capturing helper to be callable outside of an :erb invocation, however, this is a somewhat esoteric use-case, thus it might not warrant a built-in solution from nanoc. On the other hand, it's not hard to imagine someone wanting to assign captured content to an item during preprocess or within a compile rule, so perhaps it's not so esoteric after all (although the preprocess case would require an API extension to allow specifying the item for which content is being stored). Also, I haven't been able to think of a good reason why capturing should be restricted only to :erb; the restriction seems more an accident of implementation than an explicit decision.

Other local (non-nanoc) solutions are certainly possible. For instance, the imported_html filter could, rather than invoking conent_for itself, instead emit :erb code along with the body content which would indirectly invoke content_for. For instance:

class ImportedHtml < Nanoc::Filter
  identifiers :imported_html

  def run(content, params={})
    # Grab <style> and <script> from <head>, and <body> content.
    ...
    '<% content_for(:head) do %>' +
    styles + scripts +
    '<% end %>' +
    '<div>' + body + '</div>'
  end
end

with corresponding compilation rule:

compile '/README.*' do
    filter :imported_html
    filter :erb
    layout '/default.*'
    write '/documentation/index.html'
end

which isn't too bad, though the extra indirection of having one filter write code for another filter is somewhat ugly, thus the question of whether this is something nanoc should support directly.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Jan 16, 2017

Member

What would you think of allowing

content_for(:head, 'some string')

as an alternative to

content_for(:head) { _erbout << 'some string' }

? That’d fit your use case, I believe.

Member

ddfreyne commented Jan 16, 2017

What would you think of allowing

content_for(:head, 'some string')

as an alternative to

content_for(:head) { _erbout << 'some string' }

? That’d fit your use case, I believe.

@sunshineco

This comment has been minimized.

Show comment
Hide comment
@sunshineco

sunshineco Jan 16, 2017

Contributor

That would fit the use-case. As long as client code doesn't have to know implementation details of capturing (such as the nasty _erbout hack in my example), I don't personally have a strong preference between:

content_for(:head, 'string')

and:

content_for(:head) { 'string' }

However, introduction of this new method signature is rather orthogonal to the loosening of the restriction that capturing can only be used within the context of :erb. If I understand your proposal correctly, the latter form would be allowed only within an :erb invocation, which places burden on callers to choose the correct method signature depending upon calling context. Although the current implementation of content_for(:head) { 'string' } may require :erb, nothing about the method signature requires or even suggests that it should work only with :erb. As such, it might be cleaner to update the underlying implementation of the block form to work outside of :erb rather than introducing new API for that case.

Contributor

sunshineco commented Jan 16, 2017

That would fit the use-case. As long as client code doesn't have to know implementation details of capturing (such as the nasty _erbout hack in my example), I don't personally have a strong preference between:

content_for(:head, 'string')

and:

content_for(:head) { 'string' }

However, introduction of this new method signature is rather orthogonal to the loosening of the restriction that capturing can only be used within the context of :erb. If I understand your proposal correctly, the latter form would be allowed only within an :erb invocation, which places burden on callers to choose the correct method signature depending upon calling context. Although the current implementation of content_for(:head) { 'string' } may require :erb, nothing about the method signature requires or even suggests that it should work only with :erb. As such, it might be cleaner to update the underlying implementation of the block form to work outside of :erb rather than introducing new API for that case.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Jan 18, 2017

Member

@sunshineco The only way #content_for can be used right now is

content_for(:head) { _erbout << 'string' }

i.e. #content_for will figure out what is appended to _erbout, remove it, and put the addition into :head. This is the only way to be able to do

<% content_for(:head) do %>string<% end %>

The return value of the block passed to #content_for is ignored. In other words, the following will not capture any content, and not output anything either:

content_for(:head) { 'string' }

A way to make this work outside of ERB could be to check whether or not _erbout is defined. If it is not, the return value of the block is used.

Member

ddfreyne commented Jan 18, 2017

@sunshineco The only way #content_for can be used right now is

content_for(:head) { _erbout << 'string' }

i.e. #content_for will figure out what is appended to _erbout, remove it, and put the addition into :head. This is the only way to be able to do

<% content_for(:head) do %>string<% end %>

The return value of the block passed to #content_for is ignored. In other words, the following will not capture any content, and not output anything either:

content_for(:head) { 'string' }

A way to make this work outside of ERB could be to check whether or not _erbout is defined. If it is not, the return value of the block is used.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Jan 18, 2017

Member

An alternate idea (not sure what I think about it) would be to introduce a from option that identifies where to take captured content from. For example:

<% content_for(:head, from: :erb) %>string<% end %>

<% content_for(:head) %>string<% end %>

To get it from the return value:

content_for(:head, from: :return) { 'string' }

… though I‘d rather infer the capturing method (erbout vs. return value) automatically.

Member

ddfreyne commented Jan 18, 2017

An alternate idea (not sure what I think about it) would be to introduce a from option that identifies where to take captured content from. For example:

<% content_for(:head, from: :erb) %>string<% end %>

<% content_for(:head) %>string<% end %>

To get it from the return value:

content_for(:head, from: :return) { 'string' }

… though I‘d rather infer the capturing method (erbout vs. return value) automatically.

@sunshineco

This comment has been minimized.

Show comment
Hide comment
@sunshineco

sunshineco Jan 18, 2017

Contributor

Right, I understand the implementation implications. My thinking all along has been that the implementation can be smart enough to figure out automatically (depending upon presence or absence of _erbout) what to do about capturing the content generated in the block. This would keep the API simple and not place extra burden on callers.

On the other hand, it may be too magical, and simply documenting that content_for(:x) {...} is specific to :erb and that content_for(:x,'string') can be used anywhere might be fine. If taking this route, then perhaps also accept content_for(item,:x,'string') which would be useful during preprocess.

Anyhow, I don't feel too strongly one way or the other. I like the idea of content_for(:x) {...} just doing The Right Thing regardless of context in which it is called; but magical behavior can also have hidden downsides which might not become apparent until some time in the (distant) future.

Contributor

sunshineco commented Jan 18, 2017

Right, I understand the implementation implications. My thinking all along has been that the implementation can be smart enough to figure out automatically (depending upon presence or absence of _erbout) what to do about capturing the content generated in the block. This would keep the API simple and not place extra burden on callers.

On the other hand, it may be too magical, and simply documenting that content_for(:x) {...} is specific to :erb and that content_for(:x,'string') can be used anywhere might be fine. If taking this route, then perhaps also accept content_for(item,:x,'string') which would be useful during preprocess.

Anyhow, I don't feel too strongly one way or the other. I like the idea of content_for(:x) {...} just doing The Right Thing regardless of context in which it is called; but magical behavior can also have hidden downsides which might not become apparent until some time in the (distant) future.

@ddfreyne ddfreyne referenced this issue Jan 19, 2017

Merged

Allow setting content_for directly #1073

3 of 3 tasks complete
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Jan 19, 2017

Member

I started working on a potential implementation in #1073. This one uses the format

content_for(:head, 'string')
Member

ddfreyne commented Jan 19, 2017

I started working on a potential implementation in #1073. This one uses the format

content_for(:head, 'string')
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Jan 22, 2017

Member

Fixed in #1073. This’ll be part of Nanoc 4.6!

Member

ddfreyne commented Jan 22, 2017

Fixed in #1073. This’ll be part of Nanoc 4.6!

@ddfreyne ddfreyne added this to the 4.6 milestone Jan 22, 2017

@ddfreyne ddfreyne closed this Jan 22, 2017

@sunshineco

This comment has been minimized.

Show comment
Hide comment
@sunshineco

sunshineco Jan 22, 2017

Contributor

Thanks for working on this, Denis. I look forward to being able to remove the ugly _erbout hack from my code.

Contributor

sunshineco commented Jan 22, 2017

Thanks for working on this, Denis. I look forward to being able to remove the ugly _erbout hack from my code.

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