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

[API] Enhance the Request class #117

Open
rsamoilov opened this issue Jan 9, 2025 · 5 comments
Open

[API] Enhance the Request class #117

rsamoilov opened this issue Jan 9, 2025 · 5 comments
Labels
good first issue Good for newcomers

Comments

@rsamoilov
Copy link
Member

Description

Compared to Rails’ Request class, Rage’s Request currently has very few methods.

We'd like to update the class to include the following methods:

  • host
  • domain
  • method
  • get?/post?/patch?/put?/delete?/head?
  • port
  • protocol
  • query_string
  • remote_ip
  • env

Tips

  • The class is defined in lib/rage/request.rb.
  • Refer to Rails Guides if you are unsure what the return value of each method should be.
  • Check the overview doc that shows how Rage's core components interact with each other.
  • Feel free to ask any questions or request help in the comments below!
@rsamoilov rsamoilov added the good first issue Good for newcomers label Jan 9, 2025
@aaoafk
Copy link

aaoafk commented Jan 14, 2025

Any reason we don't just want to compose rack behavior similar to how rails is achieving this behavior by including Rack::Request::Helpers and Rack::Request::Env?

https://api.rubyonrails.org/v8.0.1/classes/ActionDispatch/Request.html

Reviewed both those Rack modules and it looks like that's where the methods are implemented. I was able to implement host method with the following spec after including those two modules in rage/request.rb:

  fit "handles the host property of a request" do
    expect(request.host).not_to be_nil
  end

And it looks like that works

@rsamoilov
Copy link
Member Author

Huh, I guess I didn't realise it existed. Let's use these modules - it makes perfect sense.

@aaoafk
Copy link

aaoafk commented Jan 15, 2025

Ok. Just need to make sure that these are being used correctly / if im missing anything right now, still digesting the code from the rage, rails, and rack

@aaoafk
Copy link

aaoafk commented Jan 15, 2025

Do we just want to hook into rails for some of the messages that we're missing for e.g. domain is implemented in ActionDispatch::HTTP::URL ... probably some of the other methods too that my specs are failing for... although it looks like the implementation of something like domain is straight forward...

instead of a require i think we could just extract the methods that are needed too to avoid the dependency, dunno

@rsamoilov
Copy link
Member Author

We definitely don’t want to introduce the Action Dispatch dependency. And the implementation indeed seems straightforward.
https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/actionpack/lib/action_dispatch/http/url.rb#L98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants