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

Optional parameter logic breaks generated typescript interface #119

Open
eeberhard opened this issue Jan 30, 2024 · 2 comments
Open

Optional parameter logic breaks generated typescript interface #119

eeberhard opened this issue Jan 30, 2024 · 2 comments

Comments

@eeberhard
Copy link
Member

The changes in #111 added custom logic for the optional field of Parameters:

"allOf": [
{
"if": {
"properties": {
"default_value": {
"not": {
"const": null
}
}
}
},
"then": {
"not": {
"required": [
"optional"
]
}
}
},
{
"if": {
"properties": {
"parameter_type": {
"const": "state"
}
}
},
"then": {
"required": [
"parameter_state_type"
]
}
}
]

Unfortunately, this logic breaks the generation of a typescript interface from the schema when using json2ts.

Before

Typescript parameter interface generated from schema 1-0-0:

/**
 * A dynamic parameter that is publicly configurable.
 */
export interface Parameter {
    /**
     * The human-readable parameter name.
     */
    display_name: string;
    /**
     * A brief description of the parameter.
     */
    description: string;
    /**
     * The lower_snake_case parameter name as it is declared in the implementation.
     */
    parameter_name: string;
    parameter_type: ParameterType;
    parameter_state_type?: EncodedStateType1;
    /**
     * The string representation of the default parameter value, or null if the parameter has no valid default value.
     */
    default_value: string | null;
    /**
     * Specify if this parameter can be dynamically reconfigured.
     */
    dynamic?: boolean;
    /**
     * Specify if this parameter is for internal use only and should be hidden from public users.
     */
    internal?: boolean;

    [k: string]: unknown;
}

After

Generated from schema 1-1-0:

/**
 * A dynamic parameter that is publicly configurable.
 */
export type Parameter = {
    [k: string]: unknown;
} & {
    [k: string]: unknown;
};

Why does it fail?

Typescript interfaces just describe the shape of an object by indicating the type of each fields and if it is required or not. It is unable to represent more complex logic, and so the generation will fall back to some unknown object type. In the case above, it tries to represent the allOf schema property with the union of two unknown objects.

What should we do?

The intention of the logic was justified here with the following four rules:

  • If default value is null, the schema may include an optional boolean property (but this property is not required)
  • If default value is null and optional is not specified, optional is assumed to be false (user parameter value required)
  • If default value is null and optional is specified, user parameter value is or is not required depending on the state of optional
  • If default value is not null and optional is specified, it should be a schema error.

Given the limitations of typescript, we cannot fulfill all rules in the generated type declaration. But if we are lenient on the last rule and make optional an always present but optional property, then it could be represented without issue:

export interface Parameter {
  // other fields...
    /**
     * Specify if this parameter is optional (only applicable if the default value is null)
     */
  optional?: boolean;
}

The tricky part is finding out how to revise the the schema logic to give this result while still giving us the validation errors we desire when any of the rules are violated.

@domire8
Copy link
Member

domire8 commented Feb 20, 2024

What priority does that have? Is it something that should be picked up next sprint, together with allowing sequences in the frontend?

@eeberhard
Copy link
Member Author

The consequence of this issue is that our Developer Interface will continue to support only version 1-0-0 of the descriptions syntax for components and controllers, and therefore we cannot yet build out support for descriptions that indicate optional parameters with the newer schema version 1-1-0. To be clear, because the syntax change is a revision and not a breaking model change, the Developer Interface will continue to be able to parse descriptions even if they include the optional property; it will just be ignored.

It remains a backlog issue but would be nice to resolve at some point so that we can properly indicate optional parameters in the Developer Interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants