Skip to content

feat(ExecJSRenderer) add ExecJSRenderer #299

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 3 commits into from
Jun 19, 2015
Merged

feat(ExecJSRenderer) add ExecJSRenderer #299

merged 3 commits into from
Jun 19, 2015

Conversation

rmosolgo
Copy link
Member

This ExecJSRenderer was created while testing server rendering performance and I thought it might be a chance to reorganize server rendering code. In this branch:

  • ExecJSRenderer is basically a wrapper around ExecJS
  • SprocketsRenderer is an ExecJSRenderer which fetches source JS from the asset pipeline & enables "console replay"

What do you think?

Pros:

  • ExecJSRenderer was great for testing performance. I'm not sure what other use it could have but it might come in handy again.
  • ExecJSRenderer's #before_render and #after_render hooks add extensibility as requested in Decouple renderer #253 (comment)
  • It demonstrates how Renderers fit into react-rails, maybe people will make their own

Cons:

  • The split is a bit arbitrary, specifically:
    • Where should "console replay" be implemented?
    • Which renderer decides between renderToString and renderToStaticMarkup ? Right now, that's very awkward.
  • ExecJSRenderer might be a pointless exercise because react-rails depends on sprockets, why wouldn't you use SprocketsRenderer?

Anyways, curious what others think of this change!

@rmosolgo
Copy link
Member Author

no news is good news

rmosolgo pushed a commit that referenced this pull request Jun 19, 2015
feat(ExecJSRenderer) add ExecJSRenderer
@rmosolgo rmosolgo merged commit 207149f into reactjs:master Jun 19, 2015
@rmosolgo rmosolgo deleted the execjs-renderer branch June 19, 2015 16:26
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.

2 participants