Skip to content

fix(prerender) accept string props #268

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

Merged
merged 2 commits into from
May 20, 2015
Merged

fix(prerender) accept string props #268

merged 2 commits into from
May 20, 2015

Conversation

rmosolgo
Copy link
Member

My first API was shoving prerender: into the props hash, but obviously that doesn't work if props isn't a hash!

So instead, pass prerender_options along through the ServerRendering pipeline. In theory, a render could take any options.

SprocketsRenderer looks for :static, to use React.renderToStaticMarkup

@mchristen
Copy link

I tried this out and I'm running into an issue with sprockets now.

In particular

Asset was linked to from an alias rather than its exact path. Alias resolving may not be available in production.
Use "react.js" instead of "development-with-addons/react.js"

being thrown from

#<Class:0x007fee7182f188>#asset_digest_path
sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb, line 134

This is kind of confusing because it is a call to //= require react that is raising the exception

@rmosolgo
Copy link
Member Author

wow, is there any more stack trace to that?

If it's booting a renderer and trying to prerender, I guess it's coming from here: https://github.com/reactjs/react-rails/blob/master/lib/react/server_rendering/sprockets_renderer.rb#L11

But why would it work fine in the asset pipeline but not in prerender?

@rmosolgo
Copy link
Member Author

Reproduced on Sprockets 2.8 & 2.12. Oh, I misunderstood: it's coming from the asset pipeline, not from prerender!

@mchristen
Copy link

Correct, this is for the client side JS file loaded in the rails layout.

 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:134:in `asset_digest_path'
 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:85:in `compute_asset_path'
 actionview (4.2.0) lib/action_view/helpers/asset_url_helper.rb:135:in `asset_path'  
 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:100:in `asset_path'
 actionview (4.2.0) lib/action_view/helpers/asset_url_helper.rb:245:in `javascript_path'
 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:153:in `block (2 levels) in javascript_include_tag'
 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:152:in `block in javascript_include_tag'
 sprockets-rails (2.3.1) lib/sprockets/rails/helper.rb:149:in `javascript_include_tag'
 app/views/layouts/application.html.haml:33:in `_app_views_layouts_application_html_haml___1984877869646349502_70331041908800'

@rmosolgo
Copy link
Member Author

Looks like this was touched recently in sprockets-rails, double-checking here: rails/sprockets-rails#247 (comment)

I didn't notice it before because I was on sprockets 3 :S

@rmosolgo
Copy link
Member Author

@mchristen sounds like a sprockets-rails 2.3.1 issue, see this comment: rails/sprockets-rails#247 (comment)

@mchristen
Copy link

It definitely occurs prior to that version as well(at least as early as 2.2.4). When I first ran into the error I came across that issue on sprocket-rails and saw that code was touched with 2.3.1 so I updated to that version to see if the problem went away. When the problem persisted is when I made the comment above.

Either way it looks like there will be a fix shortly, thanks for the work!

@rmosolgo
Copy link
Member Author

aiiiyayay well the regression test for this particular issue passes, so i'm gonna go for it

rmosolgo pushed a commit that referenced this pull request May 20, 2015
@rmosolgo rmosolgo merged commit 077176f into reactjs:master May 20, 2015
@rmosolgo rmosolgo deleted the prerender-string-props-fix branch May 20, 2015 23:03
@rmosolgo rmosolgo mentioned this pull request May 28, 2015
6 tasks
@rmosolgo
Copy link
Member Author

@mchristen they've shipped sprockets-rails 2.3.2 & I'm able to use them together:

~/code/check-ins $ bundle show | grep 'sprockets\|react'
  * react-rails (1.0.0 9a4f6ab)
  * sprockets (2.12.3)
  * sprockets-rails (2.3.2)

Please let me know if you can confirm/deny, if we're clear with the latest sprockets-rails I want to get ready for a new version release :D

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.

3 participants