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

[Google maps] Change the autocomplete feature to use Session tokens #3401

Conversation

aivils
Copy link
Contributor

@aivils aivils commented Aug 15, 2018

No description provided.

@aivils aivils force-pushed the google-maps-change-the-autocomplete-feature-to-use-session-tokens branch from 8cd798e to 1969672 Compare August 16, 2018 05:26
@@ -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();
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@Gnito Gnito Sep 24, 2018

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

  1. we need to create token (let's asume it is "asdf")
  2. then we search predictions for user input N with token "asdf"
  3. then we search predictions for user input Ne with token "asdf"
  4. then we search predictions for user input New with token "asdf"
  5. user selects prediciton New York
  6. 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.

Copy link
Contributor

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.

Copy link
Contributor

@Gnito Gnito Oct 1, 2018

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.

Copy link
Contributor

@Gnito Gnito Oct 1, 2018

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:

  1. This is the place where predictions are fetched.
  2. It should be grouped with getDetails call: https://github.com/sharetribe/sharetribe/blob/master/client/app/utils/places.js#L65

Copy link
Contributor Author

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.

Copy link
Contributor

@Gnito Gnito Oct 1, 2018

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

Copy link
Contributor

@Gnito Gnito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check comment

@aivils aivils force-pushed the google-maps-change-the-autocomplete-feature-to-use-session-tokens branch from 1969672 to 572779e Compare October 1, 2018 13:01
@aivils
Copy link
Contributor Author

aivils commented Oct 1, 2018

@Gnito
Changes applied. It is very suspective as did not found documneted option of session token for getDetails()

Copy link
Contributor

@Gnito Gnito left a 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 👍

@aivils aivils merged commit 4d29ee8 into master Oct 3, 2018
@aivils aivils deleted the google-maps-change-the-autocomplete-feature-to-use-session-tokens branch October 3, 2018 05:09
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 this pull request may close these issues.

3 participants