Skip to content
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

Fix component hash script #1071

Merged
merged 2 commits into from Apr 24, 2018
Merged

Fix component hash script #1071

merged 2 commits into from Apr 24, 2018

Conversation

@justisb
Copy link
Contributor

justisb commented Apr 24, 2018

Currently react_component_hash['componentHtml'] does not include the js-react-on-rails-component script tag that initializes React on Rails client-side, due to an invalid string key where a symbol is expected when building the tag internally.

While this specific issue is now fixed in the PR, perhaps some of the hash results could be refactored to use .fetch where a key should always be expected?


This change is Reviewable

@justisb justisb force-pushed the justisb:component-hash-fix branch from bb99dd5 to 583e502 Apr 24, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage remained the same at ?% when pulling 583e502 on jblasco3:component-hash-fix into 77fc623 on shakacode:master.

@justin808 justin808 merged commit 961ea0c into shakacode:master Apr 24, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justin808

This comment has been minimized.

Copy link
Member

justin808 commented Apr 24, 2018

@jblasco3 Thanks! I'm releasing 11.0.3 right now.

@justin808

This comment has been minimized.

Copy link
Member

justin808 commented Apr 24, 2018

While this specific issue is now fixed in the PR, perhaps some of the hash results could be refactored to use .fetch where a key should always be expected?

Since this "common rendering" code is used in 2 places, I'm not sure that's necessary. However, I'd like to add a test that fails without this change. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.