Generate a complete Bundler.require(...) statement for plugins #6941

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@marten
Contributor
marten commented Jul 3, 2012

The issue I encountered was that Rails mountable engines didn't work
correctly with SASS and the Asset Pipeline.

In my testcase, I have an engine that specifies some asset gem in its foo.gemspec. In the lib/foo/engine.rb it requires the gem. Then application.css.scss did:

@import 'file_from_gem';

That file would not be able to be found when trying out the engine
using the dummy application (cd test/dummy && rails server).

It would all work fine when the engine is tried out using a "full" Rails application as generated with rails new fullapp. Going over the diff between the generated test dummy and the full app, I found that changing the Bundler.require statement in config/application.rb fixed the issue, even though I'm not completely sure why.

TL;DR: The change in this commit fixes that @import statements that load css found in gems work correctly in the generated test/dummy application for Rails engines.

@marten marten Generate a complete Bundler.require(...) statement for plugins
The issue I encountered was that Rails mountable engines didn't work
correctly with SASS and the Asset Pipeline. If an engine used a gem
to provide some CSS, and application.css.scss did:

    @import 'file_from_gem';

that file would not be able to be found when trying out the engine
using the dummy application.

The change in this commit fixes this, so that @import statements that
load other css found in gems work correctly.
d8fcb31
@drogus
Member
drogus commented Jul 4, 2012

Thanks for contribution!

I found that changing the Bundler.require statement in config/application.rb fixed the issue, even though I'm not completely sure why.

This is because simple require will not load assets dependencies. This file was created long before assets were added to rails.

I have 2 problems with this pull request, though:

  1. I think that it would be better to reuse whatever we use in application.rb in application. Maybe you could try to extract it along with the other things: https://github.com/marten/rails/blob/d8fcb314fb13ba1762cf1c2441d822227b0b25ed/railties/lib/rails/generators/rails/plugin_new/plugin_new_generator.rb#L299-309 (I'm not sure if you can catch both things here, if not, you could just add similar method, for example store_bundler_definition)
  2. It has no tests. Writing acceptance tests for this can be hard, so it would be best if after doing 1), you can add test that ensures that proper things is copied.
@exviva
Contributor
exviva commented Jul 4, 2012

That's weird. Since you require the "asset" gem explicitly in lib/foo/engine.rb, Bundler.require shouldn't even be necessary. Are you sure you're requiring foo/engine somewhere?

Keep in mind that .gemspec files don't support Bundler-like groups. Using Bundler.require inside test/dummy can only give you a false sense of confidence that your engine is working properly (since Bundler is looking at your engine's Gemfile), but may fail downstream (since Bundler is looking at downstream's Gemfile, and its "assets" group).

The best I could come up with, was to mimic the behaviour of Bundler.require in my lib/foo.rb:

# most gems
# ...
require 'will_paginate'

# "asset" gems
if Rails.groups('assets' => %w[ development test ]).include?('assets')
  require 'asset_sync'
  require 'compass-rails'
  require 'fancy-buttons'
  require 'oily_png'
  require 'sass-rails'
  require 'uglifier'
end

All in all, I think I'm against this patch, since it can amplify the misconception that it's a good idea to group gem dependencies inside your gem's Gemfile and let Bundler require them.

@drogus
Member
drogus commented Jul 4, 2012

All in all, I think I'm against this patch, since it can amplify the misconception that it's a good idea to group gem dependencies inside your gem's Gemfile and let Bundler require them.

This require is only for dummy app, which is used only for testing and which should behave as regular application, so we probably would like to make it as similar to freshly generated app as we can. That said, I haven't tried to reproduce this bug yet, so I will try to do it and confirm if this is the best fix.

@exviva
Contributor
exviva commented Jul 4, 2012

This require is only for dummy app, which is used only for testing and which should behave as regular application, so we probably would like to make it as similar to freshly generated app as we can.

Well...but dummy isn't anything like a real application from Bundler's perspective, because it's using the gem's Gemfile.

If the dummy app had its own Gemfile, which would have a "path" dependency on the gem, that would more accurately simulate the actual downstream usage.

@marten
Contributor
marten commented Jul 4, 2012

Well this is awkward. Yesterday I thought I had this all figured out, but now that I'm trying to cook up an example to throw online I'm no longer able to reproduce it, not even with the "work-in-progresss" commit I made yesterday on the engine where I first encountered this.

I think @exviva raises a valid point, in that giving the dummy a real Gemfile of its own would probably make it clearer where to put stuff (if only because jquery-rails could then be removed from the engine's Gemfile which removes the conflicting message that sends), but I'm not sure if that can work with the way the integration tests are run.

@drogus
Member
drogus commented Jul 5, 2012

Well...but dummy isn't anything like a real application from Bundler's perspective, because it's using the gem's Gemfile.

Ok, I agree, haven't thought about it.

@marten please let me know if you have any success in reproducing this error on a sample engine/app

@marten
Contributor
marten commented Jul 9, 2012

@drogus No, sorry, can't manage to reproduce. Close?

@drogus
Member
drogus commented Jul 11, 2012

@marten yeah, without steps to reproduce there is not much we can do. If you encounter this problem again, please feel free to reopen or create new issue.

@drogus drogus closed this Jul 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment