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

Ensure the Props Being Passed Have Been Turned Into a Hash #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BenMorganMY
Copy link
Contributor

We encountered a case where we would pass active model errors to the response. This allows objects such as ActiveModel::Errors which can be happily serialized to a hash to be passed directly into the props without calling to_hash all of the time.

If the serializer your rails app is using is not :json or :marshal, such as :message_pack, you will encounter an error when passing in an ActiveModel::Errors object since they don't respond to to_msgpack. This also fixes this case by converting the instance to a hash and then allowing our serializer to work successfully without going back to :marshal or :json.

We encountered a case where we would pass active model errors to the response. This allows objects such as ActiveModel::Errors which can be happily serialized to a hash to be passed directly into the props without calling `to_hash` all of the time.

If the serializer your rails app is using is not :json or :marshal, such as :message_pack, you will encounter an error when passing in an ActiveModel::Errors object since they don't respond to to_msgpack. This also fixes this case by converting the instance to a hash and then allowing our serializer to work successfully without going back to :marshal or :json.
@BrandonShar
Copy link
Collaborator

Thanks @BenMorganMY ! This makes sense to me. Would you be able to add a spec that demonstrates the behavior?

Also, just for my own curiosity: what are you using on the frontend that is consuming the message_pack response?

@BenMorganMY
Copy link
Contributor Author

@BrandonShar will do, I just have one more bug to post 😅

@BenMorganMY
Copy link
Contributor Author

@BrandonShar where would be best to test this guy?

@BrandonShar
Copy link
Collaborator

I think it would be great to have a context like context 'when the serializer doesn't use something jsonable' (don't quote me directly on that name 😄 ) in the rendering_spec.

I would love something that would fail without your change in place.

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