Replay console logging on client after server prerender #196
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| end | ||
| end | ||
|
|
||
| test 'react server rendering shows console output as html comment' do |
There was a problem hiding this comment.
I think, instead of doing the fragile "copy file dance," this test should just rely on a new route and jsx file in the test app. Even something super specific, like /console_example.
|
Overall, I really like this PR. |
| @@ -22,9 +23,9 @@ def self.setup!(react_js, components_js, args={}) | |||
| @@pool = ConnectionPool.new(default_pool_options.merge(args)) { self.new } | |||
| end | |||
|
|
|||
| def self.render(component, args={}) | |||
| def self.render(component, args={}, replay_console=false) | |||
There was a problem hiding this comment.
Could replay_console be a member of the args hash? That would be nice because it would keep the Ruby idiom of options-hash-last.
I think you could fetch it out with:
replay_console = !!args.delete(:replay_console)There was a problem hiding this comment.
Thanks for the feedback. I wanted to keep it out of args because that's meant for use by the React component directly. The other place that seems reasonable would be on the app.config.react object, then passing into Renderer.setup!
There was a problem hiding this comment.
A config would be good by me, then you could just turn it always-on in development and always-off in production!
As for mixing args, it's alright in my book by way of precedent in the view helper
|
Great feature! This will be a big win for day-to-day happiness! |
|
Thanks @xionon, @rmosolgo, and @vipulnsward for all the feedback and getting this into better shape. Hoping this can make it into master. I've been using this branch locally and it's been helpful for my workflow. |
|
Changes look great! 👍 |
| Other optional configuration values include: | ||
| * `react_js`: where you want to grab the javascript library from | ||
| * `component_filenames`: an array of filenames that will be requested from the asset pipeline and concatenated together | ||
| * `replay_console`: additional debugging by replaying any captured console messages from server-rendering back on the client (note: they will lose their call stack, but it can help point you in right direction) |
|
@bikegriffith added more comments. Looking forward to this. 😄 |
|
looks good to me, last call @vipulnsward @xionon ? |
|
LGTM . @bikegriffith can you squash the commits? Its good to track this as a single change to refer in future. |
bikegriffith
force-pushed
the
console_log_server_side
branch
from
6b57838 to
52b80d9
Compare
…ages during server-side prerendering to be replayed on client to assist in debugging, per reactjs#122
bikegriffith
force-pushed
the
console_log_server_side
branch
from
52b80d9 to
585963f
Compare
|
@vipulnsward I attempted to squash the commits and also pulled the latest from upstream. Let me know if I did anything weird and I'll try to correct. Thanks! |
|
Ah, I am afraid an extra commit got in. But that should be ok, since bikegriffith@585963f is a clean commit. |
|
Just got a chance to try this out locally. (It took some finagling, my app isn't prerender-ready!). I noticed this outputs a warning, too:
(Ignore the uniform notifier, that's some other library complaining) But this seems OK with me, since it's meant for development only (right?). |
|
@rmosolgo I only intended it to be used for development, so I'm less concerned EDIT: I'm seeing it now also (on React 0.13). It doesn't appear if you disable the |
|
Yep, that was 0.13. I checked out your branch so I'm not sure how that snuck in there! Maybe because react-rails/master has 0.13 now |
Replay console logging on client after server prerender
|
🎊 thanks! |
Addresses #122. Adds a new
replay_console: trueoption, to be used in conjunction withprerender: trueon thereact_componentview helper (can assist in debugging). Updated README and added integration tests.