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

Accessing pre snapshot in layout results in Circular dependency error #537

Closed
jethrogb opened this Issue Feb 23, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@jethrogb

jethrogb commented Feb 23, 2015

When I try to access the :pre compilied snapshot for an item in a layout, I get a Circular dependency error. The final :pre snapshot gets saved at https://github.com/nanoc/nanoc/blob/7a297646/lib/nanoc/base/result_data/item_rep.rb#L380 just before the layout. Is there any reason I shouldn't be accessing this snapshot at this time?

Use case layout excerpt:

<% if @item.compiled_content(snapshot: :pre).include? 'type="math/tex' then %>
<!-- include MathJax -->
<% end %>
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Feb 28, 2015

Member

Not being able to access the :pre snapshot seems like a bug to me.

First of all, to avoid the issue you’re seeing, you can use yield to get the content right before the layout is applied. For example:

<% if yield.include? 'type="math/tex' then %>

item_rep.rb, line 247 has the check !self.compiled? && is_moving; if that expression is true, an unmet dependency error will be raised, resulting in the circular dependency issue. The reasoning here is that you should not be able to access any “moving” snapshot (i.e. :pre, :post, or :last) from the item itself.

That check is accurate for the :post and :last snapshot, since these snapshots keep moving until the item is fully compiled.

For the :pre snapshot, this check can probably be relaxed, because :pre snapshots will not move again once they’re created.

So, instead of this:

if @content[snapshot].nil? || (!self.compiled? && is_moving)

…it’d probably be possible to have this:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:pre].nil?)

I don’t think there’s any reasonable situation where yield would not be usable, though.

Member

ddfreyne commented Feb 28, 2015

Not being able to access the :pre snapshot seems like a bug to me.

First of all, to avoid the issue you’re seeing, you can use yield to get the content right before the layout is applied. For example:

<% if yield.include? 'type="math/tex' then %>

item_rep.rb, line 247 has the check !self.compiled? && is_moving; if that expression is true, an unmet dependency error will be raised, resulting in the circular dependency issue. The reasoning here is that you should not be able to access any “moving” snapshot (i.e. :pre, :post, or :last) from the item itself.

That check is accurate for the :post and :last snapshot, since these snapshots keep moving until the item is fully compiled.

For the :pre snapshot, this check can probably be relaxed, because :pre snapshots will not move again once they’re created.

So, instead of this:

if @content[snapshot].nil? || (!self.compiled? && is_moving)

…it’d probably be possible to have this:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:pre].nil?)

I don’t think there’s any reasonable situation where yield would not be usable, though.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Feb 28, 2015

Member

On second thought, my proposed fix doesn’t work. This line:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:pre].nil?)

is equivalent to

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?)

which clearly shows that :pre is not handled in a special way at all, even though it should, since it is a moving snapshot: just because a :pre snapshot exists, does not mean it cannot move in the future.

The only condition under which the :pre snapshot stops moving is when a :post snapshot is present. So, the condition could be as follows:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:post].nil?)
Member

ddfreyne commented Feb 28, 2015

On second thought, my proposed fix doesn’t work. This line:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:pre].nil?)

is equivalent to

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?)

which clearly shows that :pre is not handled in a special way at all, even though it should, since it is a moving snapshot: just because a :pre snapshot exists, does not mean it cannot move in the future.

The only condition under which the :pre snapshot stops moving is when a :post snapshot is present. So, the condition could be as follows:

if @content[snapshot].nil? || ([:post, :last].include?(snapshot) && !self.compiled?) || (snapshot == :pre && @content[:post].nil?)
@jethrogb

This comment has been minimized.

Show comment
Hide comment
@jethrogb

jethrogb Mar 10, 2015

#538 is not sufficient for fixing this. The proposed logic in ItemRep::compiled_content might make sense, but it does not match up with ItemRep::layout: there is a state where the final :pre snapshot has been created but a :post snapshot does not yet exist: https://github.com/nanoc/nanoc/blob/7a297646/lib/nanoc/base/result_data/item_rep.rb#L380-398

jethrogb commented Mar 10, 2015

#538 is not sufficient for fixing this. The proposed logic in ItemRep::compiled_content might make sense, but it does not match up with ItemRep::layout: there is a state where the final :pre snapshot has been created but a :post snapshot does not yet exist: https://github.com/nanoc/nanoc/blob/7a297646/lib/nanoc/base/result_data/item_rep.rb#L380-398

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 14, 2015

Member

To clarify: the final argument doesn’t have a meaning other than “the output file can be written to disk”—it doesn’t mark the snapshot as immovable. (In hindsight, probably not the best decision, but this could be difficult to change without breaking the API.)

The recommended solution is to use yield in a layout. In helpers used in layouts, that means passing in the content:

# in the layout
<%= snippet(yield) %>

# in lib/helpers/snippet.rb or so
def snippet(content)
  if content.size > 100
    content[0..99] + ''
  else
    content
  end
end

(This is a silly example, because there’s an #excerptize function in the Text helper.)

Making compiled_content(snapshot: :pre) available in layouts might be tricky, since the logic for determining whether :pre is immovable is determined by the presence of :post, which will not be there at the time of the first layout.

I’m actually in favour of documenting the current behavior as well as the recommended approach to avoid the issue (in other words, don’t use compiled_content(snapshot: :pre) in layouts, but use yield).

Member

ddfreyne commented Mar 14, 2015

To clarify: the final argument doesn’t have a meaning other than “the output file can be written to disk”—it doesn’t mark the snapshot as immovable. (In hindsight, probably not the best decision, but this could be difficult to change without breaking the API.)

The recommended solution is to use yield in a layout. In helpers used in layouts, that means passing in the content:

# in the layout
<%= snippet(yield) %>

# in lib/helpers/snippet.rb or so
def snippet(content)
  if content.size > 100
    content[0..99] + ''
  else
    content
  end
end

(This is a silly example, because there’s an #excerptize function in the Text helper.)

Making compiled_content(snapshot: :pre) available in layouts might be tricky, since the logic for determining whether :pre is immovable is determined by the presence of :post, which will not be there at the time of the first layout.

I’m actually in favour of documenting the current behavior as well as the recommended approach to avoid the issue (in other words, don’t use compiled_content(snapshot: :pre) in layouts, but use yield).

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 14, 2015

Member

I’ll look into improving this (i.e. making compiled_content(snapshot: :pre) possible within a layout), but it’s definitely an enhancement rather than a bug fix, since the content is already available using yield.

Member

ddfreyne commented Mar 14, 2015

I’ll look into improving this (i.e. making compiled_content(snapshot: :pre) possible within a layout), but it’s definitely an enhancement rather than a bug fix, since the content is already available using yield.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 1, 2015

Member

Fixed by #538.

Member

ddfreyne commented May 1, 2015

Fixed by #538.

@ddfreyne ddfreyne closed this May 1, 2015

@jethrogb

This comment has been minimized.

Show comment
Hide comment
@jethrogb

jethrogb May 1, 2015

As I mentioned before, this is not fixed by #538. Please re-open.

jethrogb commented May 1, 2015

As I mentioned before, this is not fixed by #538. Please re-open.

ddfreyne added a commit that referenced this issue May 1, 2015

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 1, 2015

Member

@jethrogb I’m inclined to improve this in a future release rather than 3.8.0, and consider the fix that’s currently in 3.8 as good enough. Accessing the :pre content from within a layout is not particularly useful because that content is already available through yield.

Member

ddfreyne commented May 1, 2015

@jethrogb I’m inclined to improve this in a future release rather than 3.8.0, and consider the fix that’s currently in 3.8 as good enough. Accessing the :pre content from within a layout is not particularly useful because that content is already available through yield.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 1, 2015

Member

Full fix (work in progress) in #548.

Member

ddfreyne commented May 1, 2015

Full fix (work in progress) in #548.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 4, 2015

Member

Fixed by #548.

Member

ddfreyne commented May 4, 2015

Fixed by #548.

@ddfreyne ddfreyne closed this May 4, 2015

@ddfreyne ddfreyne added this to the 3.7.6 milestone May 4, 2015

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