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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Optimize Site#each_site_file (#9187)" #9357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DavidS
Copy link

@DavidS DavidS commented Apr 29, 2023

This reverts commit b2891a4.

This is a 馃悰 bug fix.

Summary

Since #9187 static files from collections don't get written out anymore.

Context

My gallery plugin injects images and thumbnails to write out as static files into collections. With the optimization from #9187, they do not get rendered anymore. Either this revert, or adding collections.each_value { |coll| coll.files.each { |doc| yield(doc) if doc.write? } } fixes the issue for me.

@DavidS DavidS force-pushed the revert-each_site_file-optimizer branch from 16c9463 to 80e0fb0 Compare April 29, 2023 17:20
@DavidS
Copy link
Author

DavidS commented Apr 29, 2023

Updated to the second option I've found to avoid the performance impact that #9187 fixed

@ashmaroli
Copy link
Member

Hello @DavidS
It's unfortunate that the released implementation doesn't play well with your plugin.

Theoretically, the change you have proposed (as per current diff) shouldn't be necessary because the static_files reference in ln#364 is supposed to be a real-time reference to all static files in the site, including those within collections.

So, the bug is either in Jekyll's site.static_files or in your generator plugin.

@@ -363,6 +363,7 @@ def each_site_file
pages.each { |page| yield page }
static_files.each { |file| yield(file) if file.write? }
collections.each_value { |coll| coll.docs.each { |doc| yield(doc) if doc.write? } }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have a lower cyclomatic complexity, what do you think?

 collections.each_value do  |coll| 
   if doc.write?
     coll.docs.each(&method(:yield))
     coll.files.each(&method(:yield)) 
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants