google / schema-dts

JSON-LD TypeScript types for Schema.org vocabulary
Apache License 2.0
860 stars 32 forks source link

Person does not inherit correctly from interfaces PersonLeaf -> PersonBase #170

Closed TJKoury closed 2 years ago

TJKoury commented 2 years ago

Not sure if this is a Typescript, bug or a schema issue, but here goes:

Importing:

import type { Person } from "schema-dts";

Throws the TypeScript error: Property 'familyName' does not exist on type 'Person'. Property 'familyName' does not exist on type 'string'.;

Clearly it is ignoring the logical 'or' in export declare type Person = PersonLeaf | Patient | string; and skipping directly to 'string'. Not sure why.

Eyas commented 2 years ago

This could be an issue with narrowing that results in a bad error message.

In general, if you don't yet have a "@type": "Person" (or "Patient") on your object that's defined as a Person, you'll get confusing errors. Can you show me a sample of the code you have and exactly where the error is?

TJKoury commented 2 years ago

Check it out here.

NAME="Pop!_OS"
VERSION="21.10"
ID=pop
ID_LIKE="ubuntu debian"
PRETTY_NAME="Pop!_OS 21.10"
VERSION_ID="21.10"
HOME_URL="https://pop.system76.com"
SUPPORT_URL="https://support.system76.com"
BUG_REPORT_URL="https://github.com/pop-os/pop/issues"
PRIVACY_POLICY_URL="https://system76.com/privacy"
VERSION_CODENAME=impish
UBUNTU_CODENAME=impish
LOGO=distributor-logo-pop-os
Node v16.13.1

This is the error I'm getting:

index.ts:4:12 - error TS2339: Property 'additionalName' does not exist on type 'Person'.
  Property 'additionalName' does not exist on type 'string'.

4     person.additionalName = "POD";
TJKoury commented 2 years ago

I referenced a solution in this pull request. I closed the pull and just published my own fork to npm because I have a deadline, but definitely open to working the issue with you.

Eyas commented 2 years ago

DigitalArsenal/schema-dts-test is a private repo, do you mind sharing access?

TJKoury commented 2 years ago

Wow, sorry about that, I literally created it just for this thread.

Eyas commented 2 years ago

Got it. Yeah, this is indeed invalid:

const podifyPerson = (person: Person) => {
    person.additionalName = "POD";
}

because a Person can absolutely be a string.

If you expect Person not to be a string, you can use Exclude<Person, string>. Which is equivalent to PersonLeaf | PatientLeaf.

There is a request somewhere to export XLeaf properties directly. My concern is that it will encourage improper use (i.e. people using PersonLeaf when they are okay with any subtype of Person).

TJKoury commented 2 years ago

That's interesting. I'm not sure in what cases Person could be a string, especially given the Schema.org definition. Can you explain where that came from?

TJKoury commented 2 years ago

I also suppose that one could sub-type it like so to reduce verbosity, at the same time that only makes sense if you are going to extend / modify it in some way:

import { Person } from "schema-dts";
import { SLIP_0044_TYPE } from "./slip_0044";

export type cryptoKeyBase = {
    //hex publicKey
    publicKey: string,
    privateKey?: string,
    keyAddress?: string,
    //https://github.com/satoshilabs/slips/blob/master/slip-0044.md
    keyType?: SLIP_0044_TYPE | number
}

export interface cryptoKey extends cryptoKeyBase {
    "@type": "CryptoKey",
}

export type PersonPublicKey = Exclude<Person, string> & {
    key: cryptoKey | Array<cryptoKey>
}
Eyas commented 2 years ago

I'm not sure in what cases Person could be a string, especially given the Schema.org definition. Can you explain where that came from?

See #37 for more details (and #19), but technically in Schema.org, all objects can be referred to as strings. But only a small subset is frequently used.

This is typically seen in nested properties, e.g. "author" and "contentLocation" on, say, an ImageObject, where a stirng is used for a Person/Place. See https://schema.org/ImageObject#eg-0021 for example

For the purposes of this library, instead of allowing strings on all objects, we opt to only allow strings on objects where cannonical Schema.org examples show them used as strings.

TJKoury commented 2 years ago

I see it. Seems more like an oversight to me, since the Person example uses Jane Doe as the name property, making Person (and others) technically recursive data structures, as the name would be presumably referencing the same entity as the parent Person object.

That is a different topic though, I understand why you made that design decision. Thanks for the explanation.