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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require any installed prawn extensions #29

Merged
merged 9 commits into from
Jun 6, 2018

Conversation

CodingAnarchy
Copy link
Contributor

@CodingAnarchy CodingAnarchy commented Jun 5, 2018

This will search the loaded Gem::Specification for anything that matches the name prawn and require it as appropriate when loading prawn-rails. This assumes that extensions, other than prawn-rails, are named in a particular fashion (such that the gem is prawn-<extension> and is required with require prawn/<extension>), but I have only tested and verified that it works with prawn-emoji and prawn-table.

Closes #28.

require 'prawn'
require 'prawn/table'
require "prawn-rails/extension"
Gem::Specification.find_all{|s| s.name =~ /prawn/}.map(&:name).each do |gem_name|
Copy link
Collaborator

Choose a reason for hiding this comment

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

the condition for find_all should probably bes.name.start_with?('prawn-')

@@ -1,6 +1,9 @@
require 'prawn'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-add this require prawn and remove require prawn from the loop.

@westonganger
Copy link
Collaborator

We need to be careful with requiring random files as it could prove to be brittle. Please verify and test with all prawn plugins and report if any cause error during require time. For a list see: https://rubygems.org/search?utf8=%E2%9C%93&query=prawn-

For Bundler, did you ensure this searches within the bundled gems only? and does not included other installed gems within the current Ruby version?

@@ -1,6 +1,9 @@
require 'prawn'
require 'prawn/table'
require "prawn-rails/extension"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-add require "prawn-rails/extension" after this loop and remove from inside the loop

@CodingAnarchy
Copy link
Contributor Author

@westonganger I've updated the PR to address your feedback. This will now only search within the bundled gems, as Bundler ensures that only the bundled gems are loaded.

I've tested with a good chunk of the more used Prawn extensions in the list on rubygems. Of those that are still supported and can be bundled with a current version of Prawn, only prawn-svg appears to have an issue being required, because it did it's own workaround to work with prawn-rails and is not required in the traditional manner.

@westonganger
Copy link
Collaborator

Also I think it will be a good idea rescue LoadError and log a message for each require in the loop just in case they fail.

@westonganger
Copy link
Collaborator

Okay this looks good enough. I'm going to merge this. Note im planning to adjust the puts to something simple instead of the exception output.

@westonganger westonganger merged commit 3ea2b30 into cortiz:master Jun 6, 2018
@westonganger
Copy link
Collaborator

v1.1.0 released which includes this feature.

@gaffneyc
Copy link
Contributor

I just discovered this as I was working on #30 and, at the very least, this seems to be a solution in search of a problem and is likely unnecessary. It could cause unwanted behavior when loading dependencies.

With the below Gemfile we can expect that prawn-rails will load it's dependencies (prawn and prawn-table). Rather than require "prawn-table" the process of loading the dependency is obscured in the loop, which make readability harder.

gem "prawn-rails"

With the below Gemfile and this PR in place, it seems expected that prawn-rails would load prawn-svg and prawn-icon. In actuality, both prawn-svg and prawn-icon are loaded by bundler and already required by the time prawn/load_prawn_plugins is required, meaning that only prawn-table is loaded by this code. Ultimately, this only plugin this ends up requiring is prawn-table since it's not loaded in any of the other layers.

gem "prawn-rails"
gem "prawn-svg"
gem "prawn-icon"

For one last example, the developer's expectation in the below Gemfile is that prawn-svg would not be loaded except by explicit require. Let's say, for example, that you only want to load prawn-svg in a background worker to reduce memory load on your web processes. With this PR in place, the developer loses their control over that loading.

gem "prawn-rails"
gem "prawn-svg", require: false

@CodingAnarchy
Copy link
Contributor Author

I can only tell you that it was an actual issue I ran into when trying to integrate prawn-emoji into loading in a standard Rails view. prawn-svg provides its own workaround for this, so when it was required it would be loaded in appropriately, but (a) this made the order of the listing in the Gemfile important and (b) would have been much more work to copy over to every prawn library needed.

I don’t think it is unreasonable for every prawn extension to be available to prawn-rails. Yes, it removes the ability to delay requiring a library until a background worker needs it, I agree. But I think the real question I would have is why you would need to do that with such relatively small libraries, and why is that worth a brittle loading workaround?

@westonganger
Copy link
Collaborator

Until I receive a report where this feature "actually" effected someone then I am inclined to keep it, since like @CodingAnarchy states, it solves a real world issue.

One idea to mitigate would be to add an alternative require method that doesn't load plugins. Example:

# Gemfile
gem 'prawn-rails' ### loads with all plugins
# or 
gem 'prawn-rail', require: ['just_prawn_rails'] ### loads with only prawn-table

@gaffneyc
Copy link
Contributor

That's fair though my gut is telling me it's fixing a symptom but not the root cause. If I have time, I'll try to recreate the issue that @CodingAnarchy was having in #28.

@westonganger
Copy link
Collaborator

@gaffneyc I would love a full review of the situation and to discuss an alternative PR if you are so inclined.

@gaffneyc
Copy link
Contributor

In adding prawn-svg I think I understand what's going on but haven't figured out why exactly or an alternative approach just yet.

Essentially the problem appears to do with the order the plug-ins are required and the use of PrawnRails::Document. If the PrawnRails::Document class is loaded before the plug-ins then it extends a version of Prawn::Document that does not include any of the plug-in methods. This PR fixes the issue by making sure they're all required before prawn-rails is fully defined.

My first thought is it may be possible to refactor out the PrawnRails::Document class completely. Alternatively, it may be possible to make sure it has all of the extensions defined.

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.

3 participants