-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Google maps] Change the autocomplete feature to use Session tokens #3401
[Google maps] Change the autocomplete feature to use Session tokens #3401
Conversation
8cd798e
to
1969672
Compare
@@ -141,8 +141,10 @@ export const maxDistance = (place) => { | |||
export const getPrediction = (location) => new Promise((resolve, reject) => { | |||
const serviceStatus = window.google.maps.places.PlacesServiceStatus; | |||
const service = new window.google.maps.places.AutocompleteService(); | |||
const sessionToken = new window.google.maps.places.AutocompleteSessionToken(); |
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.
For the session token to take any effect we need to use the same token with predictions and with place details.
The pricing goes so that any autocomplete prediction queries are free if the token is used to fetch place details once (the place detail query ends the "session"). In other words, prediction queries are grouped to place details queries with shared session token and then the only thing that costs is place details query.
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.
Yes we do. You should forward your request to the google developers :). getDetails() does not have session token. https://developers.google.com/maps/documentation/javascript/places#place_details
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.
@aivils sorry, I didn't notice your answer earlier.
Anyway, let me rephrase my point:
don't create another sessionToken for placeDetails call. You should use the same token that was created for queryPredictions.
The way sessionToken
works is that
- we need to create token (let's asume it is "asdf")
- then we search predictions for user input
N
with token "asdf" - then we search predictions for user input
Ne
with token "asdf" - then we search predictions for user input
New
with token "asdf" - user selects prediciton
New York
- then we get place details for the selected place with token "asdf"
All the character queries for new predictions and the final selection that creates a place details query create a single session if the same token is used.
If predictions (aka character queries) and place details query have different sessionToken they are priced twice.
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.
@Gnito Here we do not send a request to google for each letter that was entered. The autocomplete is handled by googles places autocomplete widget. This code is executed when a user has already selected a location.
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.
@valdis yes, I understand. This is a UX improvement for a situation where user presses "enter" instead of choosing something from the widget's dropdown.
This code is making two calls to Google Places API, one for fetching predictions and another for place details.
My point is just to group those together. It doesn't make sense to create two separate tokens since that will duplicate place details costs. It should be possible to create the token once and pass it into those functions as a function parameter or something.
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.
I tried to scan the code a bit deeper about how these call work together:
- This is the place where predictions are fetched.
- It should be grouped with getDetails call: https://github.com/sharetribe/sharetribe/blob/master/client/app/utils/places.js#L65
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.
@Gnito Yep. In abstract. It is not documented. getDetails() has not documented option similar to sessionToken. However this getDetails() does not refuse option sessionToken. I will apply the patch in hopes google developers think similar way as I think.
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.
@aivils yeah, this pricing change has been one of the worst roll-out Google has ever made. Their API examples and docs are not yet up to date, but they still started to charge 100 times more from their API usage.
Anyway, here's the only place I have found about token usage with placedetails:
https://developers.google.com/maps/documentation/javascript/reference/places-service#PlaceDetailsRequest
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.
Check comment
1969672
to
572779e
Compare
@Gnito |
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 👍
No description provided.