esamarathon / mq-events

Repository holding information about the events published
1 stars 2 forks source link

Convert schemas to TypeScript types #7

Closed opl- closed 4 years ago

opl- commented 4 years ago

Using the standardized JSON schema format allows us to convert the schemas into TypesScript types, which could ease development of other software within the organization.

In order to achieve that, however, several questions have to be answered:

How will the types be generated?

How will they be published/used?

opl- commented 4 years ago

To clarify, per zoton's request, the issue with json-schema-to-typescript and the NodeCG/SCTimerChange event is that the teamID property only exists when the value of the desc field is equal to either team_finished or team_undid_finish.

This could be described using JSON Schema with an anyOf declaration, where either desc is equal to one of the mentioned values and the teamID property is required, or where desc is equal to any of the other valid values and the teamID property doesn't exist. Here's a simplified example (you can play around with it here):

{
    "required": ["desc", "timer"],
    "properties": {
        "timer": {
            "type": "string"
        }
    },
    "anyOf": [{
        "properties": {
            "desc": {
                "type": "string",
                "enum": ["started"]
            }
        },
        "propertyNames": {
            "enum": ["desc", "timer"]
        }
    }, {
        "required": ["teamID"],
        "properties": {
            "desc": {
                "type": "string",
                "enum": ["team_finished"]
            },
            "teamID": {
                "type": "string"
            }
        },
        "propertyNames": {
            "enum": ["desc", "teamID", "timer"]
        }
    }]
}

The problem is that the package in question doesn't correctly support the anyOf declaration and so the types it generates would be inaccurate. The current schema bypasses that by simplifying the schema, but because of that the teamID property has to be explicitly declared as not undefined or TypeScript will complain (playground link):

interface SCTimerChanged {
    desc: "started" | "team_finished";
    teamID?: string;
    timer: string;
}

function isString(input: string) {}
const msg: SCTimerChanged = {desc: "team_finished", teamID: "1", timer: "1:00"};
if (msg.desc === 'team_finished') {
    // TypeScript will complain because the types don't guarantee that `teamID` will be defined:
    isString(msg.teamID);
    // Workarounds:
    if (msg.teamID) isString(msg.teamID); // Unnecessarily verbose
    isString(msg.teamID!); // Open to issues in case of future protocol changes
}

With a proper schema, the generated types would make it clear that the teamID property has to exist for certain desc values, preventing us from having to explicitly state that in our code (playground link):

type SCTimerChanged = {
    timer: string;
} & ({
    desc: "started";
} | {
    desc: "team_finished";
    teamID: string;
});
zoton2 commented 4 years ago

Ah OK I understand now, it's just as the schema was already changed I could not tell. So this is a minor issue and nothing breaking then? I think in such case we should just use json-schema-to-typescript as in all my other projects I've had success with it, and maybe it can be fixed in the future?

opl- commented 4 years ago

Yeah, it's inconvenient and imprecise, but not breaking. That said, unless something new popped up since the issue was opened, json-schema-to-typescript is most likely the best option right now.

One possible workaround we could explore would be to manually write some of the more complex types for schemas that require it.

zoton2 commented 4 years ago

I didn't fully check the typings I had generated besides a minor skim, but I use the library with NodeCG schemas all the time and didn't experience any issues yet.

zoton2 commented 4 years ago

We've now merged the related PR but there's no separate release yet besides it just being in this repository. I'll close it for now, if anyone else thinks it should be reopened feel free.