-
Notifications
You must be signed in to change notification settings - Fork 232
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
Fingerprint crashes when o.data is a string #98
base: master
Are you sure you want to change the base?
Conversation
new Bloodhound({ datumTokenizer: Bloodhound.tokenizers.whitespace, queryTokenizer: Bloodhound.tokenizers.whitespace, aprefetch: '', remote: { url: .....', prepare: function(query, settings) { settings.type = 'POST'; settings.contentType = 'application/json; charset=UTF-8'; // Data will go into POST request body, not querystring settings.data = JSON.stringify({ q: query }); return settings; } } }); In the above scenario, settings.data should go into the request body, not into querystring. The argument name-value pairs should be in JSON format (eg {"objName":"Value"}), not in querystring format (eg objName=Value). Therefore, we set settings.data = JSON.stringify({ q: query }); - a string value. However, this doesn't bode well for function fingerprint(o): $.param(o.data || {}) crashes because it does not expect a single string value.
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.
Looks good to me.
It is a bit interesting that you're using the typeahead with POST requests, since GET requests are more appropriate for fetching typeahead suggestions. However, if this is all that's required to make POST requests work too, then I suppose there's no harm in being flexible on request type.
Can I get another review from @corejavascript/collaborators please?
I have special business case using post. But I use both in my application (get,post). Skickat från min Samsung Galaxy-smartphone. -------- Originalmeddelande -------- @jlbooker approved this pull request. Looks good to me. It is a bit interesting that you're using the typeahead with POST requests, since GET requests are more appropriate for fetching typeahead suggestions. However, if this is all that's required to make POST requests work too, then I suppose there's no harm in being flexible on request type. Can I get another review from @corejavascript/collaboratorshttps://github.com/orgs/corejavascript/teams/collaborators please? You are receiving this because you authored the thread. |
//$.param(o.data || {}) crashes because it does not expect a single string value. | ||
//if o.data is string no need to call param | ||
|
||
return o.url + o.type + (o.data &&_.isString(o.data) ? o.data : $.param(o.data || {})); |
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.
Are you missing a space here? (o.data &&_.isString(o.data)
seems like it should be (o.data && _.isString(o.data)
.
new Bloodhound({
datumTokenizer: Bloodhound.tokenizers.whitespace,
queryTokenizer: Bloodhound.tokenizers.whitespace,
aprefetch: '',
remote: {
url: .....',
prepare: function(query, settings) {
settings.type = 'POST';
settings.contentType = 'application/json; charset=UTF-8';
// Data will go into POST request body, not querystring
settings.data = JSON.stringify({ q: query });
return settings;
}
}
});
In the above scenario, settings.data should go into the request body, not into querystring. The argument name-value pairs should be in JSON format (eg {"objName":"Value"}), not in querystring format (eg objName=Value).
Therefore, we set settings.data = JSON.stringify({ q: query }); - a string value.
However, this doesn't bode well for function fingerprint(o):
$.param(o.data || {}) crashes because it does not expect a single string value.