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

Removes dependency on Hashie::Mash #1279

Closed
wants to merge 2 commits into from

Conversation

arempe93
Copy link
Contributor

Inspired by #1274

I have always found the use of Hashie::Mash as a replacement to hashes in Grape to be cumbersome. In my opinion it's kind of awkward to have params be a Hash and declared(params) be a Hashie::Mash. Especially when trying to pass declared params to ActiveRecord or any gem that expects a true Hash, not a hash-like object.

I think it should be up to the user to decide whether to use Hashie

@dblock
Copy link
Member

dblock commented Feb 17, 2016

This would break a massive amount of users of Grape, I don't think removing a dependency justifies the amount of pain and suffering that we're going to introduce by this change.

Furthermore, nothing says that this change actually improves anything (eg. performance), and that's something that we would want to demonstrate with some basic benchmarks.

I would like to be able to tell Grape to make params a plain Hash, a custom Hash class that uses Hashie mixins, or a Mash. Then we can do start deprecating things.

@arempe93
Copy link
Contributor Author

While removing a dependency is nice, removing Hashie isn't really about doing that just for the sake of it.

If there's similar performance, why use an external library instead of the native Hash? When I was making the change to remove it, we aren't even using any features of Hashie native Hashes cannot do. We just package the data in it and give it back. Why should an API DSL force the use of an external Hash library when Ruby standard Hashes would work the exact same?

@arempe93
Copy link
Contributor Author

Also, I'm assuming most people using Grape only use Hashie because of it, and there are some issues mixing Hashie with the standard hash:

hashie/hashie#341
hashie/hashie#351

I end up creating a helper in my Grape projects as a work around

@dblock
Copy link
Member

dblock commented Feb 17, 2016

Ruby standard hash doesn't work the same way. It doesn't expose properties as methods and such. There're probably significant amounts of people relying on the fact that params is a Mash. I am not arguing this is a bad change, I am just saying that we need to demonstrate that it's worth it and provide an easy way for backward compatibility. Try to address this?

@arempe93
Copy link
Contributor Author

Been trying to come up with an idea to ease the transition for people using Hashie but can't seem to come up with something very elegant. Do you have any ideas @dblock?

@arempe93
Copy link
Contributor Author

Maybe we can add a configuration to transform params with a block? And the default return value would just be a plain Hash?

transform_params do |params|
  # convert to Hashie
end

Idk just spitballing....

@dblock
Copy link
Member

dblock commented Feb 25, 2016

Maybe just write a middleware someone can use?

@james2m
Copy link
Contributor

james2m commented Apr 11, 2016

@arempe93 I'm totally with you on this, I hate my code being polluted with Hashie and now we have #dig the case is even stronger. How about you set a params_hash_class somewhere and specify Hashie as default. Then reference that throughout instead of referencing Hashie.new or idiomatic Hash[] use params_hash_class.new?

@vbalazs
Copy link

vbalazs commented Jun 22, 2016

Not a fresh article about why is not a good idea depending on it:
http://www.schneems.com/2014/12/15/hashie-considered-harmful.html

I spent a few hours debugging an issue migrating from Rails 4.1 to 4.2. You can see the problem in the commit in which they reverted the behaviour for Rails 5.
rails/rails@64ae9a5
This totally broke our symbolize_keys method calls :)

I am behind removing hashie as a dependency. Instead of thinking about what it breaks (which we can handle with semantic versioning and the changelog), think about what it really provides for grape and if that's worth keeping for.

@dblock
Copy link
Member

dblock commented Jun 23, 2016

@vbalazs see above. I am looking for a PR that is green and that gives people a backwards-compatible way forward (even if they have to do a bit of work)

@arempe93
Copy link
Contributor Author

So I decided to give this another look because of the discussion around it. Basically the hurdle was getting it to work on Rails 3.x because of the missing deep_symbolize_keys methods in ActiveSupport.

I tried using i18n/core_ext/hash as a replacement, but the implementation differed from ActiveSupport (i18n version does not symbolize hashes within arrays) and caused specs to fail. So I regrettably had to do my own patch on Hash. If someone knows a better way, that would probably be preferable.

@@ -261,6 +261,7 @@ def initialize(value)
requires :b
end
end
subject.before_validation { p params }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug code leftover ...

@dblock
Copy link
Member

dblock commented Jun 25, 2016

This passes the build, so it's on the right track. I think we could kill the Rails 3 support in this PR if it makes things a lot easier, it's about time, no? :)

We still need a path for everyone forward, and I mean a way to get a Hashie::Mash with a switch kind of thing or a middleware, changelog and readme and upgrading updates, etc.

@msroot
Copy link

msroot commented Jun 30, 2016

What about using a ActiveSupport::HashWithIndifferentAccess ?

@dblock
Copy link
Member

dblock commented Jul 1, 2016

@msroot I am open to it, we already depend on AS.

@jcope2013
Copy link

@vbalazs what was your solution on rails 4.2 to get around that PR only being available on rails 4.1/rails 5? monkey patching?

@vbalazs
Copy link

vbalazs commented Aug 16, 2016

@jcope2013 i was in a situation where i could just do grape_params.to_h and dealing with params as simple a hash from there.

@jcope2013
Copy link

@vbalazs thanks for the suggestion

@dblock
Copy link
Member

dblock commented Apr 9, 2017

Closing in favor of #1610. Please add your comments!

@dblock dblock closed this Apr 9, 2017
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.

6 participants