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

fix(geocoder): Propogate geocoder error to consumer #487

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

prateek3255
Copy link

Description

  • Adds a new prop to propagate errors from the Geocoder.geocode API to the consumer application
  • Use the new @types/google.maps library for google maps types (The old one was deprecated)
  • Adds minor modification to the types to incorporate the newer types changes

closes #486

Checklist

  • All tests passing
  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary
  • Commits and PR follow conventions

closes ubilabs#486

Add a new prop to propogate errors from the Geocoder.geocode API to the consumer application
@akashnimare
Copy link

@yfr hey, can you review this when you get the time? Our Sentry is full of these errors and thought it would be great to have a fix in the upstream.

Copy link
Member

@yfr yfr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. We had a lot on our plates but i managed to review now.

Just two small changes.

Comment on lines +545 to 546
const location = (gmaps?.geometry &&
gmaps.geometry.location) as google.maps.LatLng;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const location = (gmaps?.geometry &&
gmaps.geometry.location) as google.maps.LatLng;
const location = gmaps?.geometry?.location as google.maps.LatLng;

Comment on lines +577 to +578
const location = (gmaps?.geometry &&
gmaps.geometry.location) as google.maps.LatLng;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const location = (gmaps?.geometry &&
gmaps.geometry.location) as google.maps.LatLng;
const location = gmaps?.geometry?.location as google.maps.LatLng;

@plumdumpling
Copy link
Contributor

plumdumpling commented Aug 3, 2023

@prateek3255 would you like to update this pull request according to @yfr's comments, so we can get this done?

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.

Handle the errors returned by the geocoder API
4 participants