Conversation
|
Tests only fail because of the the same issue failing all recent react_on_rails tests: something in the HelloWorldApp used by the Basic Example. I think it's likely https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/templates/dev_tests/spec/features/hello_world_spec.rb#L10, but I have yet to confirm this. |
|
Generally LGTM, just few comments... Review status: 0 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. spec/dummy/app/views/pages/broken_app.html.erb, line 1 at r1 (raw file):
broken_app name seems too abstract here (while BrokenApp component name is ok). I would express that we're demonstrating hash result behavour in template name. spec/dummy/spec/features/integration_spec.rb, line 200 at r1 (raw file):
Should we also add a "black" test for case when we retruning a hash but Comments from Reviewable |
|
Reviewed 11 of 11 files at r1. Comments from Reviewable |
I'd like to see "it just works" behavior. Is there possibly a super clear choice as to which is behavior is the only one we need? Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. spec/dummy/app/views/pages/broken_app.html.erb, line 1 at r1 (raw file): Previously, udovenko (Denis Udovenko) wrote…
I agree with @udovenko. spec/dummy/spec/features/integration_spec.rb, line 200 at r1 (raw file): Previously, udovenko (Denis Udovenko) wrote…
I agree with @udovenko. But we first need to decide why we need both cases...Maybe a little doc would clarify? Comments from Reviewable |
| server_rendered_html = result["html"] | ||
|
|
||
| if result["hasErrors"] && options.hash_result | ||
| server_rendered_html = Hash[COMPONENT_HTML_KEY, result["html"]] |
There was a problem hiding this comment.
prefer normal hash literal syntax:
server_rendered_html = { COMPONENT_HTML_KEY => result["html"] }| end | ||
|
|
||
| def hash_result | ||
| retrieve_key(:hash_result) |
There was a problem hiding this comment.
Please update the retrieve_key implementation as follows:
def retrieve_key(key)
options.fetch(key) do
if ReactOnRails.configuration.methods.include?(key)
ReactOnRails.configuration.public_send(key)
else
nil
end
end
endThis will allow you to remove the hash_key option from the initializer configuration and keep it as only an option you can pass to react_component directly. Previously, the assumption had been made that every option passed to react_component had a fallback default in the initializer, but in this scenario, that would make no sense and it's unnecessarily expanding the public API of the initializer.
| replay_console: true, | ||
| logging_on_server: true, | ||
| raise_on_prerender_error: false, | ||
| hash_result: false, |
There was a problem hiding this comment.
See comment for react_component/options.rb, we should remove all of this
|
It was decided that a documentation PR would address the concerns of this PR without the need to add yet another option parameter to |
When I created #951, I forgot to add the useful functionality from #950 that would ensure that react_component_hash worked correctly. When there is a prerendering error, react_component_hash should return the result combined with the console logs as a hash even though every prerendering error is returned from Exec as a String. Right now, if a prerendering error occurs, react_component_hash throws its "Expected a Hash" error
4 participants
Expected behavior: If the user signals ReactOnRails that the user expects
react_componentto return a Hash (which is currently done through https://github.com/shakacode/react_on_rails/pull/800/files#diff-04c6e90faac2675aa89e2176d2eec7d8R343), thenreact_componentshould return a Hash or nil in all cases.Actual behavior: The current signal for
react_componentto return a Hash requires successful rendering. Any error in the react code will result in the signal being lost andreact_componentreturning a string.This change is