Skip to content

Conversation

@vipulnsward
Copy link
Contributor

  • Assets version and register_engine have been moved from app.assets to app.config.assets on sprockets
  • Added a check to make sure to use proper asset object when calling version, register_engine, etc
…to app.config.assets on sprockets - Added a check to make sure to use proper asset object when calling version, register_engine, etc
Copy link
Contributor

Choose a reason for hiding this comment

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

(app.assets || Sprockets).register_engine '.jsx', React::JSX::Template``` Maybe this way?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually its harder to read that way and make the intent clear why we are doing it.

Copy link
Member

Choose a reason for hiding this comment

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

I also like the idea of removing that duplication, maybe:

sprockets_env = app.assets || Sprockets # Sprockets 3.x expects this in a different place  sprockets_env.register_engine(".jsx", React::JSX::Template)

IMO less duplication = easier to maintain in the future!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@vipulnsward
Copy link
Contributor Author

Hi @rmosolgo, can you take a look at this when you time. This causes issues when used with newest version of sprockets.

Copy link
Member

Choose a reason for hiding this comment

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

I think this could also be DRYer, eg

sprockets_env = app.assets || app.config.assets # sprockets-rails 3.x attaches this at a different config sprockets_env.version = [ ... ].compact.join("-")
@vipulnsward
Copy link
Contributor Author

@rmosolgo done.

@rmosolgo
Copy link
Member

🎊 nice! maybe we should add an Appraisal for Rails 5 beta or something next.

rmosolgo pushed a commit that referenced this pull request Jul 23, 2015
Fix running on sprockets-rails master
@rmosolgo rmosolgo merged commit c189f27 into reactjs:master Jul 23, 2015
@rovr
Copy link

rovr commented Sep 2, 2015

@rmosolgo, @vipulnsward Just a heads up, I've been trying to make it work with Rails 5 and atm it immediately fails on .register_engine line.

Looks like sprockets removed engines completely.

So with sprockets 4.0 the initializer should probably look something like:

 sprockets_env.register_mime_type 'text/jsx', extensions: ['.jsx', '.js.jsx'] sprockets_env.register_transformer 'text/jsx', 'application/javascript', React::JSX::Template 
@vipulnsward
Copy link
Contributor Author

🙈 Sprockets is unstable atm with all the refactoring. I will work on
fixing this against master and keep it backwards compatible.

On Wednesday, September 2, 2015, rovr notifications@github.com wrote:

@rmosolgo https://github.com/rmosolgo, @vipulnsward
https://github.com/vipulnsward Just a heads up, I've been trying to
make it work with Rails 5 and atm it immediately fails on .register_engine
line.

Looks like sprockets removed engines completely
rails/sprockets@37a1e24
.

So with sprockets 4.0 the initializer should probably look something
like:

sprockets_env.register_mime_type 'text/jsx', extensions: ['.jsx', '.js.jsx'] sprockets_env.register_transformer 'text/jsx', 'application/javascript', React::JSX::Template 


Reply to this email directly or view it on GitHub
#322 (comment).

Vipul A.M.
+91-8149-204995

@rovr
Copy link

rovr commented Sep 2, 2015

@vipulnsward Thanks! I understand, I just thought I might save you some debugging time in the future.

So if sprockets choose to go with the current interface:

The object that you pass to the register_transformer method must implement a call method.

So if you do

sprockets_env.register_transformer 'text/jsx', 'application/javascript', React::JSX

Then inside of React::JSX you must have a method like:

 def self.call(code) self.transformer ||= transformer_class.new(transform_options) self.transformer.transform(code[:data]) end 

Looks like that's all it takes to make it work with the current sprockets master branch.

Here's an example implementation.

@jondot
Copy link

jondot commented Sep 5, 2015

Bumping into this now. if this helps @rovr's fork works for me with current Rails master - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants