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 running on sprockets-rails master #322

Merged
merged 3 commits into from
Jul 23, 2015

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
Sprockets.register_engine '.jsx', React::JSX::Template
else
app.assets.register_engine '.jsx', React::JSX::Template
end
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.

if app.assets.nil?
app.config.assets.version = [app.config.assets.version, "react-#{asset_variant.react_build}",].compact.join('-')
else
app.assets.version = [app.assets.version, "react-#{asset_variant.react_build}",].compact.join('-')
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants