Skip to content

feat(ExecJSRenderer) add ExecJSRenderer#299

Merged
rmosolgo merged 3 commits intoreactjs:masterfrom
rmosolgo:execjs-renderer
Jun 19, 2015
Merged

feat(ExecJSRenderer) add ExecJSRenderer#299
rmosolgo merged 3 commits intoreactjs:masterfrom
rmosolgo:execjs-renderer

Conversation

@rmosolgo
Copy link
Copy Markdown
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
Copy Markdown
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