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.

Sign up for GitHub

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

Jump to bottom

Remove cookie-jar dependency for ruby 3.3 #2395

Merged
merged 8 commits into from
Dec 31, 2023
Merged

Remove cookie-jar dependency for ruby 3.3 #2395

merged 8 commits into from
Dec 31, 2023

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented Dec 29, 2023 •
edited
Loading

Fix #2394

This PR also changes our cookie[...].delete data. I followed Rack::Utils.delete_set_cookie_header. At some point, we should refactor our Grape::Cookies to use the function instead of explicitly setting the values.

Documentation about Max-Age

@ericproulx
Remove cookie-jar dependency
0567128 
Remove deleted and set max-age 0 for current cookies
Replace HTTP_COOKIE by set-cookie function
Replace Set-Cookie by Rack::SET_COOKIE
Update specs
@ericproulx
Remove delete_set_cookie_header and set_cookie_header since its in ra…
b349c12 
…ck >= 3.0
@ericproulx
Add cookie_helper to manage different expires value.
1dc9799
@ericproulx
Fix rubocop
77f2cec
@ericproulx
Copy link
Contributor Author

I haven't found a good candidate to replace cookiejar, so I've created a helper to manage common cases and different version of rack. Speaking of rack, we might consider dropping rack < 2 since we dropped Rails 5.2 and Ruby 2.6. Starting from Rails 6, it's rack >=2.

@ericproulx
Copy link
Contributor Author

@dblock something is going on with danger

bundler: failed to load command: danger (/home/runner/work/grape/grape/vendor/bundle/ruby/2.7.0/bin/danger)
/home/runner/work/grape/grape/vendor/bundle/ruby/2.7.0/gems/octokit-4.25.1/lib/octokit/response/raise_error.rb:14:in `on_complete': GET https://api.github.com/repos/ruby-grape/grape/pulls/2395: 403 - API rate limit exceeded for user ID 20649811. If you reach out to GitHub Support for help, please include the request ID 2848:17B1:4FF5E1:A82AE0:658ED9FE. // See: https://docs.github.com/rest/overview/rate-limits-for-the-rest-api (Octokit::TooManyRequests)

dblock
dblock approved these changes Dec 30, 2023
View reviewed changes
Copy link
Member

@dblock dblock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks nice and clean and extensible! Do update CHANGELOG even if it's test work.

@ericproulx ericproulx marked this pull request as ready for review December 30, 2023 17:31
@ericproulx
Copy link
Contributor Author

This looks nice and clean and extensible! Do update CHANGELOG even if it's test work.

Done

dblock
dblock reviewed Dec 31, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dblock dblock merged commit 15c891c into ruby-grape:master Dec 31, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ruby 3.3 && cookiejar = ArgumentError
2 participants
@ericproulx @dblock