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

Require `runtime_dependencies` of a Gem-based theme from its `.gemspec` file #5914

Merged
merged 7 commits into from Mar 31, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Feb 28, 2017

Have Jekyll require the runtime_dependencies of a theme-gem from its .gemspec file so that the theme's consumers need not have to edit their gems array to include plugins required by their theme.

--
/cc @jekyll/ecosystem

@ashmaroli ashmaroli changed the title from Theme deps to Require `runtime_dependencies` of a Gem-based theme from its `.gemspec` file Feb 28, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 1, 2017

Member

Requesting a restart of the Travis build for this PR..
Thanks 😃

Member

ashmaroli commented Mar 1, 2017

Requesting a restart of the Travis build for this PR..
Thanks 😃

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 1, 2017

Member

I couldn't load the page to trigger a restart... Try pushing another commit?

Member

parkr commented Mar 1, 2017

I couldn't load the page to trigger a restart... Try pushing another commit?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 1, 2017

Member

Try pushing another commit?

okay..

Member

ashmaroli commented Mar 1, 2017

Try pushing another commit?

okay..

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 4, 2017

Member

label: themes

Member

ashmaroli commented Mar 4, 2017

label: themes

@DirtyF DirtyF added the themes 🎨 label Mar 4, 2017

@parkr

Seems fine to me.

Show outdated Hide outdated lib/jekyll/plugin_manager.rb
end
dependencies.each do |dep|
External.require_with_graceful_fail(dep.name) unless dep.name == "jekyll"
end

This comment has been minimized.

@parkr

parkr Mar 7, 2017

Member

You could simplify this to

site.theme.runtime_dependencies.each do |dep|
  next if dep.name == "jekyll"
  External.require_with_graceful_fail(dep.name) if plugin_allowed?(dep.name)
end
@parkr

parkr Mar 7, 2017

Member

You could simplify this to

site.theme.runtime_dependencies.each do |dep|
  next if dep.name == "jekyll"
  External.require_with_graceful_fail(dep.name) if plugin_allowed?(dep.name)
end
Show outdated Hide outdated lib/jekyll/plugin_manager.rb
@@ -29,6 +30,15 @@ def require_gems
)
end
def require_theme_deps
if site.theme.runtime_dependencies
site.theme.runtime_dependencies.each do |dep|

This comment has been minimized.

@parkr

parkr Mar 9, 2017

Member

We tend to prefer guard block style, which is:

def require_theme_deps
  return false unless site.theme && site.theme.runtime_dependencies
  # ... do the actual work, expecting the above variables to exist
end

You could alternatively use #Array(), which will create an empty array if the value is nil:

def require_theme_deps
  Array(site.theme.runtime_dependencies).each do |dep|
    # ...
  end
end
@parkr

parkr Mar 9, 2017

Member

We tend to prefer guard block style, which is:

def require_theme_deps
  return false unless site.theme && site.theme.runtime_dependencies
  # ... do the actual work, expecting the above variables to exist
end

You could alternatively use #Array(), which will create an empty array if the value is nil:

def require_theme_deps
  Array(site.theme.runtime_dependencies).each do |dep|
    # ...
  end
end

This comment has been minimized.

@ashmaroli

ashmaroli Mar 10, 2017

Member

I gez I'll go with the explicit return style instead of the alternative wherein a block is called even on an empty array.

@ashmaroli

ashmaroli Mar 10, 2017

Member

I gez I'll go with the explicit return style instead of the alternative wherein a block is called even on an empty array.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 9, 2017

Member

@pathawks Any objections to the idea here?

Member

parkr commented Mar 9, 2017

@pathawks Any objections to the idea here?

@parkr parkr added this to the 3.5 milestone Mar 9, 2017

@pathawks

Seems reasonable 👍

Show outdated Hide outdated docs/_docs/themes.md
From `v3.5`, Jekyll will automatically require all `whitelist`-ed `runtime_dependencies` of your theme-gem even if they're not explicitly included under the `gems` array in the site's config file. (NOTE: `whitelist`-ing is only required when `build`-ing or `serve`-ing with the `--safe` option.)
With this, the end-user need not keep track of the plugins required to be included in their config file for their theme-gem to work as intended.

This comment has been minimized.

@ashmaroli

ashmaroli Mar 10, 2017

Member

Added a piece of documentation for this particular feature.
/cc @DirtyF

@ashmaroli

ashmaroli Mar 10, 2017

Member

Added a piece of documentation for this particular feature.
/cc @DirtyF

This comment has been minimized.

@DirtyF

DirtyF Mar 10, 2017

Member

@ashmaroli Could we please stick to plain english instead of mixing commands and dashes to ease readability?

@DirtyF

DirtyF Mar 10, 2017

Member

@ashmaroli Could we please stick to plain english instead of mixing commands and dashes to ease readability?

This comment has been minimized.

@ashmaroli

ashmaroli Mar 10, 2017

Member

Sure.
Something like this: ?

From `v3.5`, Jekyll will automatically require all whitelisted `runtime_dependencies` of
your theme-gem even if they're not explicitly included under the `gems` array in the
site's config file. (NOTE: whitelisting is only required when building or serving with the
`--safe` option.)

With this, the end-user need not keep track of the plugins required to be included in their
config file for their theme-gem to work as intended.
@ashmaroli

ashmaroli Mar 10, 2017

Member

Sure.
Something like this: ?

From `v3.5`, Jekyll will automatically require all whitelisted `runtime_dependencies` of
your theme-gem even if they're not explicitly included under the `gems` array in the
site's config file. (NOTE: whitelisting is only required when building or serving with the
`--safe` option.)

With this, the end-user need not keep track of the plugins required to be included in their
config file for their theme-gem to work as intended.
@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Mar 17, 2017

Member

I just realized that this may have an unintended side-effect:
Users won't be able to disable a certain plugin if they wish to.

Scenario:
Say, a theme listed jekyll-feed as a dependency. But the user would like to use another plugin to generate their feeds. There's now no way to do so..


UPDATE:

Two possible solutions:

  1. The user should exclude jekyll-feed from the whitelist: and run the site in safe mode.
  2. We implement a blacklist: to allow the user to explicitly disable jekyll-feed and run the site in normal mode.
Member

ashmaroli commented Mar 17, 2017

I just realized that this may have an unintended side-effect:
Users won't be able to disable a certain plugin if they wish to.

Scenario:
Say, a theme listed jekyll-feed as a dependency. But the user would like to use another plugin to generate their feeds. There's now no way to do so..


UPDATE:

Two possible solutions:

  1. The user should exclude jekyll-feed from the whitelist: and run the site in safe mode.
  2. We implement a blacklist: to allow the user to explicitly disable jekyll-feed and run the site in normal mode.
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Mar 17, 2017

Member

Themes should only require what they require.

If a user wants something lighter weight, they can either use a lighter theme, or fork a theme and remove the cruft.

Member

pathawks commented Mar 17, 2017

Themes should only require what they require.

If a user wants something lighter weight, they can either use a lighter theme, or fork a theme and remove the cruft.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 31, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 0eb9379 into jekyll:master Mar 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 31, 2017

@ashmaroli ashmaroli deleted the ashmaroli:theme-deps branch Mar 31, 2017

@tlnagy

This comment has been minimized.

Show comment
Hide comment
@tlnagy

tlnagy Jul 20, 2017

I agree with @ashmaroli here.

I just realized that this may have an unintended side-effect:
Users won't be able to disable a certain plugin if they wish to.

Here's my use case. I developed a theme for jekyll (https://github.com/tlnagy/jekyll-lab-notebook) that is more of a theme+engine. It's an electronic lab notebook engine (for biological sciences, mainly) that is a nice theme + a bunch of plugins (https://github.com/tlnagy/jekyll-lab-notebook-plugins) to make everything work well. I wanted it to be easy to set up so the theme requires the plugins gem, but now there is no way to disable the plugins. The blacklist option sounds like it could work for my situation.

tlnagy commented Jul 20, 2017

I agree with @ashmaroli here.

I just realized that this may have an unintended side-effect:
Users won't be able to disable a certain plugin if they wish to.

Here's my use case. I developed a theme for jekyll (https://github.com/tlnagy/jekyll-lab-notebook) that is more of a theme+engine. It's an electronic lab notebook engine (for biological sciences, mainly) that is a nice theme + a bunch of plugins (https://github.com/tlnagy/jekyll-lab-notebook-plugins) to make everything work well. I wanted it to be easy to set up so the theme requires the plugins gem, but now there is no way to disable the plugins. The blacklist option sounds like it could work for my situation.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Jul 21, 2017

Member

Hi @tlnagy the consensus is that a theme should henceforth list only the very essential plugins (i.e. those without which, your theme would simply fail) as runtime_dependencies. The non-essential ones can be brought to the end-user's notice with effective documentation.
They're then free to add the non-essential plugins to their config file as required.

Member

ashmaroli commented Jul 21, 2017

Hi @tlnagy the consensus is that a theme should henceforth list only the very essential plugins (i.e. those without which, your theme would simply fail) as runtime_dependencies. The non-essential ones can be brought to the end-user's notice with effective documentation.
They're then free to add the non-essential plugins to their config file as required.

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