-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix: registration during the checkout flow #1684
Conversation
} | ||
|
||
Client.prototype.get = function(uri, data, options = {}) { | ||
Client.prototype.get = function(uri, options = {}) { |
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.
What if we want to pass GET query parameters?
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.
We will need to find a workaround for it. I remember while debugging another issue I noticed that either on Android or iOS GET requests are rejected if they include a body. And we already ignore this parameter for GET requests and only add it to POST and PUT requests: https://github.com/coopcycle/coopcycle-app/blob/master/src/API.js#L146-L159 That's why I thought it will be cleaner to remove it for now, rather than spreading {}
in the method calls across the whole project
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.
It would be cool to keep the same method signature IMHO. For GET query parameters, Axios accepts the params
key in the options object. In createRequest
, we could do this:
if (method.toUpperCase() === 'GET') {
req.params = params
}
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.
Axios would take care of things like URL encoding.
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.
In my opinion, it's very confusing if for GET requests we start mapping the data field to params. I'd be happy to keep the same method signature, but if data
field is not applicable to GET requests, I think, it's better to make it explicit, rather than give an impression that it works the same as for other requests.
# Conflicts: # src/redux/App/selectors.js # src/redux/reducers.js
No description provided.