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

Lucky::RequestTypeHelpers#ajax? always false #1131

Closed
skojin opened this issue May 2, 2020 · 3 comments · Fixed by #1134
Closed

Lucky::RequestTypeHelpers#ajax? always false #1131

skojin opened this issue May 2, 2020 · 3 comments · Fixed by #1134

Comments

@skojin
Copy link
Contributor

skojin commented May 2, 2020

enable :js format in action: accepted_formats [:js]
do remote request with rjs
ajax? returns false because clients_desired_format == :js

This happen because accept_header checked before X-Requested-With

if usable_accept_header? && accept
from_accept_header(accept)
elsif ajax?
:ajax

@paulcsmith
Copy link
Member

Ohh that makes sense. Would you mind writing a PR with a spec that catches this and then put ajax? ahead of the usable_accept_header?

@skojin
Copy link
Contributor Author

skojin commented May 6, 2020

Ok,

But the question is why they are exclusive? I can make xhr request and it will be both :js and :ajax

I can't just move line just ahead of usable_accept_header, that will give page format :ajax instead of :js

I suggest remove this spec

it "returns :ajax if it is an AJAX request and no 'Accept' header is given" do
format = determine_format("X-Requested-With": "XmlHttpRequest")
format.should eq(:ajax)
end

and make request_type_helpers.cr#ajax? just check XMLHttpRequest, same as rails xhr?

@paulcsmith
Copy link
Member

@skojin You are totally right. Removing that spec and making ajax? just check the header sounds perfect. I imagine it'd still be nice to combine "accepts" and "ajax" together and your proposed approach makes that easy 👍

skojin added a commit to skojin/lucky that referenced this issue May 7, 2020
Use ajax? in redirect helper instead of inline check

Fixes luckyframework#1131
paulcsmith pushed a commit that referenced this issue May 12, 2020
Use ajax? in redirect helper instead of inline check

Fixes #1131
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 a pull request may close this issue.

2 participants