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

Ambiguous TS Definitions #305

Closed
nolawnchairs opened this issue Oct 27, 2020 · 1 comment · Fixed by #306
Closed

Ambiguous TS Definitions #305

nolawnchairs opened this issue Oct 27, 2020 · 1 comment · Fixed by #306
Labels
good first issue Good for newcomers

Comments

@nolawnchairs
Copy link

This applies to version 1.6.0

The definitions for the City result contains a union type with empty objects that conflict with VSCode intellisense. Note that I'm using the City object as the example, but I see this same issue with other model type definitions::

export default class City extends Country {
    readonly city: records.CityRecord | {};
    readonly location: records.LocationRecord | {};
    readonly postal: records.PostalRecord | {};
    readonly subdivisions: records.SubdivisionsRecord[] | [];
    constructor(response: CityResponse);
}

As you can see, when trying to use the resultant reader value, the editor shows the following errors:

Screenshot from 2020-10-15 14-53-11

This is because the TS engine assumes the lowest-possible denominator of a union type, so it assumes the empty object {} instead of CountryRecord. The only way around this is to cast the object to a type or interface that removes the union types:

import { Reader, CountryRecord, SubdivisionsRecord, CityRecord, PostalRecord, LocationRecord, TraitsRecord } from '@maxmind/geoip2-node'
import ReaderModel from '@maxmind/geoip2-node/dist/src/readerModel'

interface GeoIpResult {
  country: CountryRecord,
  subdivisions: SubdivisionsRecord[],
  city: CityRecord,
  postal: PostalRecord,
  location: LocationRecord,
  traits: TraitsRecord
}

const { country, subdivisions, city, postal, location, traits } = reader.city(ip) as GeoIpResult

This allows for normal usage, but is a completely unnecessary step. I should also note that ReaderModel should be exported (defined) in your main index module, so importing from @maxmind/geoip2-node/dist/src/readerModel would not become necessary.

There is also an error with the SubdivisionsRecord array, as it's defined.

readonly subdivisions: records.SubdivisionsRecord[] | [];

There is no need to union with an empty array type, as this will hint to the compiler that's an empty array of nothing as opposed to an empty array of SubdivisionsRecord. All this serves to do is erase the type hints for the resultant array. If you're returning an empty array, it can remain the type of the expected array, since it won't be iterated anyways.

readonly subdivisions: records.SubdivisionsRecord[]

It would be better to remove this union type all together and do one of the following, assuming that one of these record sets would not contain a result:

  1. Make all properties of nullable types
export interface CityRecord {
    readonly confidence?: number;
    readonly geonameId?: number;
    readonly names?: Names;
}
  1. Make the record itself nullable and return null or undefined if it does not exist, instead of an empty object
export default class City extends Country {
    readonly city?: records.CityRecord;
    readonly location?: records.LocationRecord;
    readonly postal?: records.PostalRecord;
    readonly subdivisions?: records.SubdivisionsRecord[];
    constructor(response: CityResponse);
}

The latter of course, would introduce breaking changes but would conform more to JavaScript/NodeJS conventions.

@kevcenteno kevcenteno added the good first issue Good for newcomers label Oct 28, 2020
@kevcenteno
Copy link
Member

You're right, we'll take a closer look at this.

@kevcenteno kevcenteno mentioned this issue Oct 30, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

2 participants