google-maps-services-js icon indicating copy to clipboard operation
google-maps-services-js copied to clipboard

Exported `Place` type has fully optional properties

Open SpencerKaiser opened this issue 3 years ago • 6 comments

Every single property belonging to Place is optional when that isn't practical in reality and puts the developer in a really awkward position when things that should always be present can be undefined.

Environment details

  1. client.placeDetails > PlaceDetailsResponse > PlaceDetailsResponseData > Place
  2. macOS 12.1 (21C52)
  3. "@googlemaps/google-maps-services-js": "^3.3.6",, node v16.13.0

Steps to reproduce

  1. Use client.placeDetails within a typescript app
  2. Examine the type of the saved response's Place within data.result

OR

See this exported type that marks all properties as optional...

Code example

interface SimplePlaceDetails {
  address: string;
  latitude: number;
  longitude: number;
  rating?: number;
  numReviews?: number;
}

// ...

const client = new Client({});

const response = await client.placeDetails({
  params: {
    place_id: placeId,
    sessiontoken: sessionToken,
    key: env.googleApiKey,
  },
});

const { result } = response.data;
const { lat: latitude, lng: longitude } = result.geometry?.location ?? {};
return {
  address: result.formatted_address,
  latitude,
  longitude,
  rating: result.rating,
  numReviews: result.reviews?.length,
};

Compilation error (abbreviated): Type '{ address: string | undefined; latitude: number | undefined; longitude: number | undefined; rating: number | undefined; numReviews: number | undefined; }' is not assignable to type 'SimplePlaceDetails'.

Literally the entirety of Place is a partial so every single one of its properties can be undefined. I get this for certain values like ratingandreviewsbut it makes zero sense for things likeformatted_address, lat, lng`, etc. unless the docs are updated to provide context for when they may actually be undefined.

I need to get lat and long for my app to function properly and right now I'm going to have to make an assumption that it's defined and just throw an error otherwise, which kinda sucks...

Can someone take a pass at cleaning that type up and making sure that only legitimately optional params are marked as being optionals? I'm totally onboard with logically optional things being optional, but either devs need good docs to understand when things may be undefined or the type needs to be cleaned up a bit.

Thank you!!!

SpencerKaiser avatar Jan 29 '22 20:01 SpencerKaiser

@SpencerKaiser Thank you for opening this issue. 🙏 Please check out these other resources that might be applicable:

jpoehnelt avatar Jan 29 '22 20:01 jpoehnelt

every single property belonging to Place is optional when that isn't practical in reality

all fields can be made optional through the fields parameter. i'm not aware of any way to automatically use the fields param and Pick/Required or similar to change this.

You can do this in your code with some combination of Pick and Required. the following sets all fields as required and then picks only a few of those.

result = result as Pick<Required<Place>, "geometry" | "formatted_address">>;

I'm going to close this as there is no further action that I see for this library with the current features of TypeScript.

jpoehnelt avatar Jan 29 '22 23:01 jpoehnelt

Couldn't you make the fields param keyof Place and then make the return type an Omit of Place and fields?

Seems like making literally everything optional because of the fields param is sort of cutting corners especially when the docs don't specify what data will be returned based on the type of location; for example, reviews will never be returned for a home address, right? (This is a question because the docs mention nothing...) If anything, I'd make the return type Place | Partial<Place> and let the user asset the response based on whether they provided fields.

I really don't think this issue should have been closed, especially given there was no feedback from others in this community or from other maintainers.... this really feels like a cop-out to avoid writing decent types for the SDK...

SpencerKaiser avatar Jan 31 '22 04:01 SpencerKaiser

I was able to get the value of fields to Pick the correct fields from Place.

I'll clean up the types and make a pull request to see if that is something that y'all are ok adding before I do the work to add it to all functions using a fields option @jpoehnelt

IMG_0526

johnkahn avatar Jan 31 '22 19:01 johnkahn

Note that fields can also be of the form geometry/location and are not strictly keysof Place.

jpoehnelt avatar Feb 01 '22 01:02 jpoehnelt

@johnkahn A pr would be great.

jpoehnelt avatar Feb 01 '22 15:02 jpoehnelt