Require theme dependencies #19
Conversation
| @@ -9,16 +9,43 @@ class MockGemspec | |||
| extend Forwardable | |||
| def_delegator :theme, :root, :full_gem_path | |||
|
|
|||
| DEPENDENCY_PREFIX = %r!^\s*[a-z]+\.add_(?:runtime_)?dependency! | |||
There was a problem hiding this comment.
Hm. I wish we could just use _config.yml, since we can load that safely. What is the advantage of using the gemspec? What if a remote theme doesn't specify a gemspec but does specify a _config.yml? Could we try both?
There was a problem hiding this comment.
I could see it either way. If you're a theme, you'd then have to list dependencies twice. That said, if you're using e.g., the page gem, you'd have to list it twice in your gemspec.
Do you think it's more intuitive to look to the plugins key of the theme? Will that cause confusion when other values aren't carried over? FWIW, the example theme doesn't have a config file.
There was a problem hiding this comment.
If they build an example site, they’ll specify a _config.yml because Pages doesn’t parse the gemspec. While developing, they may want to use a config file instead of a gemspec for simplicity. So I’d say doing both—and this mimicking the normal jekyll experience—would be best. Reading in the theme config would be interesting in general!
There was a problem hiding this comment.
The jekyll-data plugin reads and loads a config file within a theme-gem when available.. and merges that data into the original site.config hash.. perhaps have something like that here too?
| output, status = Open3.capture2e(*args) | ||
| output = output.encode("UTF-8", | ||
| @output, @status = Open3.capture2e(*args(config_path)) | ||
| @output = @output.encode("UTF-8", |
There was a problem hiding this comment.
Can we do output in a let(:output) block instead of this? let is generall preferred over instance variables in RSpec that I've seen.
There was a problem hiding this comment.
Let isn't intended for values that persist between examples. This allows us to build the site once, rather than per test.
|
|
||
| # Hash in the form of gemspec fixture => expected dependencies | ||
| { | ||
| "alldeps" => %w(jekyll jekyll-feed jekyll-sitemap bundler rake), |
There was a problem hiding this comment.
bundler and rake are usually development dependencies. We don't want to require those, right?
There was a problem hiding this comment.
In the fixture, they're listed as runtime.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@benbalter Hello! Want us to pick this back up? I love the idea... |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I'd like to move forward with merging this. I'm going to stick with the Gemspec-based approach, since that's what themes do today, but see value in a config-based approach (either in addition or instead) down the line. |
This PR builds on @ashmaroli's work over in #15 by folding the PluginManager logic into MockGemspec so that we can use Jekyll's native PluginManager functionality to safely require theme dependencies.
Specifically:
MockGemspec#runtime_dependenciesnow returns an array ofGem::Dependencyobjects, which it builds from a regex match against the theme's Gemspecsite.plugin_manager.require_theme_depsinMunger#configure_theme, which callstheme.gemspec.runtime_dependenciesand uses Jekyll's native theme dependency logic to require only whitelisted plugins, provides error output, etc.Since this is potentially requiring arbitrary ruby, @parkr, I'd ❤️ your 👀 as well, when you can.