-
Notifications
You must be signed in to change notification settings - Fork 115
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 cookies from incoming request are passed to Grover via Middleware #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable idea @braindeaf however I'm not super keen on rolling a custom cookie parser. Suggest look at using something like Rack::Utils.parse_cookies_header
as used by Rack::Request. ie
cookies = Rack::Utils.parse_cookies_header(env[Rack::HTTP_COOKIE]).map do |key, value|
{ name: key, value: value, domain: env[Rack::HTTP_HOST] }
end
Noted, will make appropriate changes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks fine, although the build is failing because of the middleware file length being too long.
As a temporary measure, update https://github.com/Studiosity/grover/blob/main/.rubocop.yml#L26 to 140
Also, can you please add an entry in the unreleased section of CHANGELOG.md
syntax changes Add to Unreleased CHANGELOG
Done. I'm getting some rubocop errors with bundled Rubocop 0.81. I didn't want to go and upgrade it in this pull, but happy to if need be. Error: unrecognized cop Layout/SpaceAroundMethodCallOperator found in .rubocop.yml, unrecognized cop Style/ExponentialNotation found in .rubocop.yml |
No worries, I'll look at those in a separate PR |
FYI I've released this in 0.12.2. Thanks for the PR 👍 |
I came across this situation this week, where I was loading a page that had javascript based calendar which loaded additional resources. While the page loaded the JSON that populated the calendar did not because the AJAX calls to fetch data did not have the cookies it needed.
This just reuses the example of re-sending cookies, but it puts it into the middleware.