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

Support sprockets-rails3 #1

Merged
merged 5 commits into from
Jan 14, 2016
Merged

Support sprockets-rails3 #1

merged 5 commits into from
Jan 14, 2016

Conversation

yasaichi
Copy link
Owner

@yasaichi yasaichi commented Jan 2, 2016

I got the following error when I used this gem with sprockets-rails v3.0.0.

/home/travis/build/yasaichi/gakubuchi/lib/gakubuchi/template_engine.rb:26:in `registered?': undefined method `engines' for nil:NilClass (NoMethodError)
    from /home/travis/build/yasaichi/gakubuchi/lib/gakubuchi/template_engine.rb:17:in `register!'
    from /home/travis/build/yasaichi/gakubuchi/lib/gakubuchi/railtie.rb:4:in `block in <class:Railtie>'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/railties-4.2.5/lib/rails/initializable.rb:30:in `instance_exec'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/railties-4.2.5/lib/rails/initializable.rb:30:in `run'
    from /home/travis/.rvm/gems/ruby-2.2.0/gems/railties-4.2.5/lib/rails/initializable.rb:55:in `block in run_initializers'

(You can see the full stack trace here.)

As far as I looked over sprockets-rails, it occurred due to the difference of how to define Rails.application.assets.

In v2.x, it is defined as an instance method of Rails::Application class (ref. here).
Therefore, we can access Sprockets::Environment in initializer blocks.

On the other hand, in v3.0.0, Rails.application.assets is defined by attr_accessor (ref. here).
Then, the value is set in the config.after_initialize block (ref. here).
Therefore, we can access Sprockets::Environment only in config.after_initialize blocks.

In addition, from the latest version, Rails.application.assets always returns nil when config.assets.compile is disabled (ref. README.md).

For these reasons, we must add the following changes to support sprokets-rails3.

  • Use config.assets.configure instead of initializer.
  • Use other methods than ::Rails.application.assets.find_asset to resove digest paths of templates

@yasaichi yasaichi self-assigned this Jan 2, 2016
@yasaichi yasaichi force-pushed the support-sprockets-rails3 branch 2 times, most recently from ae31b8b to 3b65d99 Compare January 3, 2016 03:54

def register!(target, engine)
klass = constantize(engine)
return false if !klass || registered?(target)
Copy link
Owner Author

Choose a reason for hiding this comment

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

[IMO]
klass.instance_of?(::Class) seems better.

@yasaichi yasaichi force-pushed the support-sprockets-rails3 branch from 3b65d99 to 5148272 Compare January 3, 2016 13:15
subject { -> { described_class.new(source_path) } }

context 'when source path is relative' do
let(:source_path) { 'foo.html' }
Copy link
Owner Author

Choose a reason for hiding this comment

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

[IMO]
foo.html.erb seems better because in most cases the format of a source file is something other than html.

@yasaichi yasaichi changed the title [WIP] Support sprockets-rails3 Support sprockets-rails3 Jan 3, 2016
@yasaichi yasaichi changed the title Support sprockets-rails3 [WIP] Support sprockets-rails3 Jan 4, 2016
::Rails.public_path.join(logical_path)
end

def digest_path
Copy link
Owner Author

Choose a reason for hiding this comment

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

[IMO]
Related to this comment, you should implement this method as follows.

def digest_path_in_context(context)
  # do something
end

@yasaichi yasaichi force-pushed the master branch 2 times, most recently from 17083e5 to d9816f1 Compare January 10, 2016 09:07
@yasaichi yasaichi force-pushed the support-sprockets-rails3 branch 5 times, most recently from c4853cc to 8fa12b1 Compare January 13, 2016 14:24
@yasaichi yasaichi force-pushed the support-sprockets-rails3 branch from 8fa12b1 to 5deb445 Compare January 13, 2016 15:10
@yasaichi yasaichi changed the title [WIP] Support sprockets-rails3 Support sprockets-rails3 Jan 14, 2016
yasaichi added a commit that referenced this pull request Jan 14, 2016
@yasaichi yasaichi merged commit f669b68 into master Jan 14, 2016
@yasaichi yasaichi deleted the support-sprockets-rails3 branch January 14, 2016 15:50
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.

1 participant