Skip to content

Feature Request: Use non-optional properties in types #29

Open
@qwrwed

Description

Is your feature request related to a problem? Please describe.
I've been using the types from this module - they've been very useful, so thank you. Lately I've been having to work around the optional properties (example). TypeScript gives me errors such as:

Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
Type 'undefined' is not assignable to type 'string'.

Object is possibly 'undefined'.

Type 'undefined' cannot be used as an index type.

This is because every property (that I've seen) is optional, which means that something which should be type e.g. string is actually inferred to be type string | undefined, requiring use of type casts (modeName as string), non-null assertions, and/or optional chaining operators (stopPointsOnLines?.[modeName!]?.[line]).

Do they need to be optional? In my experience, the TFL API always returns something for each field, even if that's just an empty value (e.g. empty list), and if some field is missing, we might not want to just continue as though there were no issues. There may be some optional fields in TFL's spec, but I feel as though these would be the exception, rather than the rule.

Describe the solution you'd like
I propose making most, if not all, of the properties non-optional, so e.g. TfL['Mode'] would go from:

Mode: {
        isTflService?: boolean;
        isFarePaying?: boolean;
        isScheduledService?: boolean;
        modeName?: string;
    };

to

Mode: {
        isTflService: boolean;
        isFarePaying: boolean;
        isScheduledService: boolean;
        modeName: string;
    };

This would mean that instead of e.g. (property) isTflService?: boolean | undefined, we'd just have (property) isTflService: boolean, instead of needing to implement some way of telling TS it shouldn't be undefined for each property.

Describe alternatives you've considered
Workarounds as above, but this feels somewhat hacky/clunky.

Additional context
This information is based on my experience of using VSCode and create-react-app on Windows.

image

Activity

changed the title Use non-optional properties in types Feature Request: Use non-optional properties in types on Jan 31, 2022
ZackaryH8

ZackaryH8 commented on Jan 31, 2022

@ZackaryH8
Owner

The only problem with this is that the types are auto-generated from the swagger file, this means that I would have to reimplement custom types.

I will take a look at your react application as soon as I can get it to compile

qwrwed

qwrwed commented on Jan 31, 2022

@qwrwed
ContributorAuthor

Swagger file, for reference

Couple of questions:

  1. How is this used to generate the types?
  2. At which point does the "optional" aspect get introduced?

re: compiling my app, is there anything in particular giving you trouble?

ZackaryH8

ZackaryH8 commented on Feb 1, 2022

@ZackaryH8
Owner
  1. How is this used to generate the types?

I don't remember the exact name but there is a TypeScript tool

  1. At which point does the "optional" aspect get introduced?

I assume it's when the types are actually generated

re: compiling my app, is there anything in particular giving you trouble?

No problems now, all sorted!

qwrwed

qwrwed commented on Feb 1, 2022

@qwrwed
ContributorAuthor

I've been having a look around [1] [2] [3] and it looks like the response definitions may be missing the required field in TfL's Swagger file.

Edit: seems fields are optional unless declared as required [4] [5] [6]. Looks like the problem is indeed with TfL's Swagger file.

qwrwed

qwrwed commented on Feb 4, 2022

@qwrwed
ContributorAuthor

Looks like this might have been the tool: https://www.npmjs.com/package/openapi-typescript
It also looks like the swagger file might be outdated (see here (dates erroneously in query instead of path, like in the swagger file) vs here (dates in path as intended - doesn't match the file)). I'm just going to make my own version to use.

ZackaryH8

ZackaryH8 commented on Feb 4, 2022

@ZackaryH8
Owner

Reopened to track adding custom types back, instead of using the swagger file to generate them, they will be maintained in this repo. (This is how it was done previously).

self-assigned this
on Feb 4, 2022
ZackaryH8

ZackaryH8 commented on Feb 8, 2022

@ZackaryH8
Owner
export module GetByIDs {
    export interface Root {
        naptanId: string;
        modes: string[];
        icsCode: string;
        stopType: string;
        stationNaptan: string;
        lines: Line[];
    }

    export interface Line {
        $type: string;
        id: string;
        name: string;
        uri: string;
        type: string;
        routeType: string;
        status: string;
    }
}
import * as IStopPoint from './interfaces/stopPoint'

getByIDs(ids: Array<string>, includeCrowdingData: boolean): Promise<Array<IStopPoint.GetByIDs.Root>> {
    return this.sendRequest(`/StopPoint/${TfLAPI.arrayToCSV(ids)}`, { includeCrowdingData }, 'GET');
}

This is how the types were done before, but I'm pretty sure it abuses TypeScript doing it this way. I thought about just removing all the optional properties from the current setup, do you think this is viable solution?

qwrwed

qwrwed commented on Feb 11, 2022

@qwrwed
ContributorAuthor

I'm quite new to TypeScript, so I'm not yet qualified to say whether that's good practice or not.

What do you mean by removing all optional properties - just removing the ? after each property? That would be a viable solution (assuming that all responses should contain all properties - which I think is a valid assumption).

I think it would work, but I also think it would be much better to have a fixed Swagger file to generate the TS from. I started work on one here, and I've also found this one from 5 years ago (haven't looked at it yet). I've also just now created a thread on the API forums to see if any better Swagger file can be made available.

qwrwed

qwrwed commented on Feb 12, 2022

@qwrwed
ContributorAuthor

I think adding required: [{field1}, {field2}, ...] to each definition should work - the change I made in this example, along with the same change for a couple of other definitions, has rendered the corresponding TypeScript definitions non-optional.
image

This should work as a solution, though it will require some refactoring - Line's Tfl-3 definition is different from Journey's Tfl-3 definition, so each API (line, journey etc) would have to be stored in different files.

I've written a couple of quick scripts - one to add the requirements (input/*.json -> output/*.json via convert.js) and one to convert to TS (output/*.json -> ts/*.ts via tsconvert.bat) at https://github.com/qwrwed/tfl-openapi-typescript-fix. I've added the outputs as well; feel free to implement them or provide feedback.

Edit: I've also added a script to create a JSON map (see here) of the new type names (only those that are named Tfl-[n]) and corresponding old type names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions

    Feature Request: Use non-optional properties in types · Issue #29 · ZackaryH8/tfl-api-wrapper