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

fix for sprockets-3.0.x #143

Merged
merged 1 commit into from Apr 17, 2015
Merged

fix for sprockets-3.0.x #143

merged 1 commit into from Apr 17, 2015

Conversation

masarakki
Copy link
Contributor

after sprockets-3.0.x, I can't find depend_on files when assets.paths dosen't include directory.

@le0pard
Copy link
Member

le0pard commented Apr 13, 2015

Hello, @masarakki . Thanks for pull request. But also need fix tests, because https://github.com/rails/sprockets/blob/v3.0.0/lib/sprockets/context.rb also changed in sprockets 3.0.

@masarakki
Copy link
Contributor Author

How can I write spec for both 3.0.x and 2.x? use Appraisal?

@le0pard
Copy link
Member

le0pard commented Apr 13, 2015

Yes, you can use Appraisal and also you can write conditions inside specs (you have access to Sprockets::VERSION, like Sprockets::VERSION =~ /\A3\.\d+\.\d+\z/)

@le0pard
Copy link
Member

le0pard commented Apr 13, 2015

Thanks @masarakki, this pull request looks like good. Need review from @bogdan (I think better remove 1.9.3 from CI matrix - it is already unsupported).

@@ -2,9 +2,13 @@ class JsRoutes
class Engine < ::Rails::Engine
JS_ROUTES_ASSET = 'js-routes'

initializer 'js-routes.append_assets_path', group: :all, after: :append_assets_path do |app|
app.config.assets.paths.unshift Rails.root.join('config').to_s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to add config to assets path? Can you describe with details?

Copy link
Member

Choose a reason for hiding this comment

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

@masarakki
Copy link
Contributor Author

depend_on of Sprockets 3.0.x check the requested file is under the assets_paths

https://github.com/rails/sprockets/blob/master/lib/sprockets/resolve.rb#L69

@le0pard
Copy link
Member

le0pard commented Apr 14, 2015

Hello @masarakki .

I think we don't need add

initializer 'js-routes.append_assets_path', group: :all, after: :append_assets_path do |app|
  app.config.assets.paths.unshift Rails.root.join('config').to_s
end

to assets_paths, because we render js-routes from https://github.com/railsware/js-routes/blob/master/app/assets/javascripts/js-routes.js.erb file, which is located in assets_path (I hope). Does it work without this?

@pbhogan
Copy link

pbhogan commented Apr 14, 2015

It is technically necessary since sprockets checks that all dependencies are themselves under the asset paths and routes.rb is not. However, adding config to the asset paths is not good in my opinion. It may have other unwanted side effects.

A mechanism seems to exist to declare external environmental dependencies in sprockets 3. I've taken a shot at implementing using it myself and this seems to work:
https://github.com/pbhogan/js-routes/blob/master/lib/js_routes/engine.rb

I've left the original register_preprocessor call in since it is necessary for sprockets 2 but is just rejected by sprockets 3 harmlessly in depend_on here:
https://github.com/sstephenson/sprockets/blob/v3.0.0.beta.8/lib/sprockets/resolve.rb#L65

@masarakki
Copy link
Contributor Author

@pbhogan
it seems good!!

@masarakki
Copy link
Contributor Author

update code.


# only sprockets >= 3.0
if Rails.application.assets.respond_to?(:depend_on)
Rails.application.assets.depend_on "file-digetst://#{routes}"
Copy link
Member

Choose a reason for hiding this comment

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

Little typo: file-digest:// should be.

@masarakki
Copy link
Contributor Author

oh, thx!

@le0pard
Copy link
Member

le0pard commented Apr 15, 2015

waiting @bogdan

FileUtils.rm_f(NAME)
after(:each) do
FileUtils.rm_rf Rails.root.join('tmp/cache')
FileUtils.rm_f NAME
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think remove those files in before is enough. Why do you think after makes sense?

@bogdan
Copy link
Collaborator

bogdan commented Apr 15, 2015

Looks good in general. Only one small comment regarding rspec callbacks.

@le0pard
Copy link
Member

le0pard commented Apr 17, 2015

ping @bogdan :)

bogdan added a commit that referenced this pull request Apr 17, 2015
@bogdan bogdan merged commit caa3aae into railsware:master Apr 17, 2015
@masarakki masarakki deleted the fix-for-sprockets-3.0.x branch April 17, 2015 09:01
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.

None yet

4 participants