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

Add Oj to Ruby On Rails #4508

Merged
merged 2 commits into from
Mar 5, 2019
Merged

Add Oj to Ruby On Rails #4508

merged 2 commits into from
Mar 5, 2019

Conversation

johnvuko
Copy link
Contributor

@johnvuko johnvuko commented Mar 5, 2019

Hi,
I made a little change to the configuration of the Ruby On Rails framework.
You are using the default serializer of Ruby which is known to not be performant.
The Oj gem is not provided out of the box with Rails but it's a must have that every Rails developers have installed.
It's not an opinatied choice, there are only 2 major known / used serializers in Ruby, the default of Ruby and Oj.

Regards,
John

@NateBrady23
Copy link
Member

@jonathantribouharet We've recently enforced this requirement in #4291

It looks like rails is one of those frameworks that is caching the date header. I doubt the gem you added is responsible for this, but it won't pass until that's taken care of.

@johnvuko
Copy link
Contributor Author

johnvuko commented Mar 5, 2019

The problem is config.action_dispatch.default_headers.merge!('Date' => Time.now.httpdate, 'Server' => 'WebServer') in config/application.rb, this line set a header, so the header 'Date' will always be the same value.
I have two questions:

  • what is the usage / goal of this header?
  • do you want me to fix it in this PR or another?

@NateBrady23
Copy link
Member

@jonathantribouharet In this PR is fine, as we don't want to merge anything new until it passes the travis tests. The headers are just required as part of the tests:

  • The response headers must include either Content-Length or Transfer-Encoding.
  • The response headers must include Server and Date.

We generally don't do the test implementations; we leave that up to the community. So feel free to adjust as you see fit. Thanks!

@NateBrady23
Copy link
Member

Nice work! Thanks for the updates!

@NateBrady23 NateBrady23 merged commit 47f6c11 into TechEmpower:master Mar 5, 2019
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