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-rails ~> 3.2 #54

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

obliviusm
Copy link

@obliviusm obliviusm commented Dec 27, 2016

Hello. Let me explain myself

Problem: New version of sprockets-rails is not compatible with princely. In new version of sprockets-rails code Rails.application.assets returns assets for development environment but doesn't return assets in production-like environments.
Solution: I've modified AssetSupport#asset_file_path. So it generates the sprockets environment if it finds Rails.application.assets to be nil.
This solution is taken from Compass/compass-rails#263 . They had the same problem.

In this PR I've done next:

  • added dummy rails project that is used in test
  • added development dependencies: rails, sprockets-rails
  • added tests for AssetSupport#asset_file_path that checks production environment. If you run this test with the old version of AssetSupport#asset_file_path you'll see that it fails.
  • modified AssetSupport#asset_file_path as described in solution

Thanks

@pftg
Copy link

pftg commented Dec 28, 2016

@obliviusm pls fix broken tests

@repinel
Copy link
Contributor

repinel commented Jul 7, 2017

Not sure about the issue/fix, but having a dummy Rails app to test is definitely something good to have.

@modsognir What do you think if I create a new PR just to add the Rails app test for the current code in master? Later, this PR would be simpler to review.

@pftg
Copy link

pftg commented Jul 7, 2017

@repinel there is dummy app in this pr

@repinel
Copy link
Contributor

repinel commented Jul 7, 2017

@repinel there is dummy app in this pr

@pftg Yes, I noticed. I was basically suggesting separating this PR in two.

@modsognir
Copy link
Collaborator

Sorry I missed this one folks. @repinel yes, if the dummy app was split into a seperate PR that would make this one a lot easier to check over. Not sure why this is failing on 1.9.3 but I think we might be able to remove that as 1.9.3 hasn't been officially supported in 2 years.

@repinel
Copy link
Contributor

repinel commented Aug 1, 2017

@modsognir I can work on it

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.

5 participants