Skip to content

Follow sproket engine interface guidelines#70

Merged
jhilden merged 2 commits intozweilove:masterfrom
nickd0:sprokets_interface_fix
Aug 22, 2016
Merged

Follow sproket engine interface guidelines#70
jhilden merged 2 commits intozweilove:masterfrom
nickd0:sprokets_interface_fix

Conversation

@nickd0
Copy link
Contributor

@nickd0 nickd0 commented Jul 25, 2016

https://github.com/rails/sprockets/blob/master/guides/extending_sprockets.md#supporting-all-versions-of-sprockets-in-processors

new.render on Sprokets engines is deprecated as of Sprokets v3.7.0 (its currently called through the Sprockets::LegacyTiltProcessor class - see sprockets-3.7.0/lib/sprockets/legacy_tilt_processor.rb:20).

This PR implement the class method call which instantiates a CssSplitter::SproketEngine and calls render.

@jhilden
Copy link
Contributor

jhilden commented Jul 25, 2016

thanks @nick-donald I'm assuming that this is a fix for #69

could @adzap and @bhalash please try this out and report if it works?

is thery anything that we could/should add a test for?

data = input[:data]
context = input[:environment].context_class.new(input)

data = self.new(filename) { data }.render(context, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

i find this syntax a little hard to understand. could this be expressed in a more simple way?

@nickd0
Copy link
Contributor Author

nickd0 commented Jul 25, 2016

@jhilden Clarified and added comments.

As stated above this is a fix for #69

I don't think this needs additional tests since this is .call is only every called by Sprokets. And since #evaluate is left intact, it should be fully backwards compatible.

@jhilden
Copy link
Contributor

jhilden commented Jul 27, 2016

could we get some feedback from @adzap and @bhalash on this, so I can merge this?

@adzap
Copy link

adzap commented Aug 4, 2016

@jhilden looks good. Fixes deprecations.

@oboxodo
Copy link

oboxodo commented Aug 15, 2016

Any plans to finally merge this and release a new gem version? It would be pretty helpful.

@jhilden jhilden merged commit 24017c1 into zweilove:master Aug 22, 2016
@jhilden
Copy link
Contributor

jhilden commented Aug 22, 2016

Sorry for the delay, since I'm on vacation. PR is merged an release should follow in the next couple of days. Thanks for testing the PR.

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.

4 participants