googlemaps / google-maps-services-js

Node.js client library for Google Maps API Web Services
Apache License 2.0
2.88k stars 634 forks source link

Exported `Place` type has fully optional properties #750

Open SpencerKaiser opened 2 years ago

SpencerKaiser commented 2 years ago

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 likeratingandreviewsbut 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!!!

jpoehnelt commented 2 years ago

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

jpoehnelt commented 2 years ago

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.

SpencerKaiser commented 2 years ago

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...

johnkahn commented 2 years ago

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

jpoehnelt commented 2 years ago

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

jpoehnelt commented 2 years ago

@johnkahn A pr would be great.