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

Chore/remove rails from required libraries #12

Merged
merged 6 commits into from
Sep 20, 2019

Conversation

EmCousin
Copy link
Owner

@EmCousin EmCousin commented Sep 18, 2019

This PR will close #11

  • Removing an unnecessary call to rails. We're keeping Rails as a development dependency at the moment
  • Fixed potential security issue with Nokogiri
  • Provides a more intuitive ability to use a custom serializer when rendering an object

@spijet
Copy link

spijet commented Sep 18, 2019

I see a weird error when sending a hash with an empty array inside as a response — the app catches a NoMethodError in the Formatter class. Is it expected?
Please check this gist for minimal app code and log.

@EmCousin
Copy link
Owner Author

@spijet Interesting. After digging a bit it appears that a forked version fixed that: cartedo@79049c1

I'll see for updating the PR with adapted testing cover

@spijet
Copy link

spijet commented Sep 18, 2019

Yep, this seems to fix the problem with empty arrays.

I also ran into a problem with fast_jsonapi_serializable() method, but it's caused by my own stuff — the app relies on module namespacing, so all serializer classes are named as AppName::Serializers::<Model> instead of <Model>Serializer. I'll probably just rewrite the method to make it work with this scheme and bundle it as an extended class. :)

@spijet
Copy link

spijet commented Sep 18, 2019

Actually, I was wrong, and my app does use <Model>Serializer classes, but it still holds them in AppName::Serializers:: namespace. So, here's what worked for me in the end:

def serializable_class(object)
  object = object.first if object.is_a?(Array)
  klass = object.model_name.name.split('::').last
  format('%<klass>sSerializer', klass: klass).safe_constantize
end

It probably won't work for everyone, as it (most likely) relies on auto-loading and auto-resolving magic made possible by Zeitwerk gem, but at least it won't fail on non-namespaced classes.

@spijet
Copy link

spijet commented Sep 18, 2019

A little addition to my take on serializable_class() — in case ActiveSupport is available, we can use ActiveSupport::Inflector.demodulize method to make it cleaner. But its usefulness is questionable, since it does almost the same thing (that is, drops everything up until after last :: in given String object). :)

```ruby
get "/" do
user = User.find("123")
render user, serializer: 'CustomUserSerializer'
Copy link

Choose a reason for hiding this comment

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

This is exactly what I wanted to propose later today! :D Thank you so much!!

@spijet
Copy link

spijet commented Sep 20, 2019

I have converted my app to use Grape and grape_fast_jsonapi for its API and it works perfectly in production environment. Thank you so much! :)

@EmCousin EmCousin merged commit aa1f410 into master Sep 20, 2019
@EmCousin EmCousin deleted the chore/remove-rails-from-required-libraries branch September 20, 2019 08:06
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.

Is Rails dependency really necessary?
2 participants