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

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

Open
qwrwed opened this issue Jan 31, 2022 · 9 comments
Open

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

qwrwed opened this issue Jan 31, 2022 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@qwrwed
Copy link
Contributor

qwrwed commented Jan 31, 2022

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

@qwrwed qwrwed changed the title Use non-optional properties in types Feature Request: Use non-optional properties in types Jan 31, 2022
@ZackaryH8
Copy link
Owner

ZackaryH8 commented Jan 31, 2022

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

@ZackaryH8 ZackaryH8 added the enhancement New feature or request label Jan 31, 2022
@qwrwed
Copy link
Contributor Author

qwrwed commented Jan 31, 2022

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
Copy link
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
Copy link
Contributor Author

qwrwed commented Feb 1, 2022

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
Copy link
Contributor Author

qwrwed commented Feb 4, 2022

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.

@qwrwed qwrwed closed this as completed Feb 4, 2022
@ZackaryH8 ZackaryH8 reopened this Feb 4, 2022
@ZackaryH8
Copy link
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).

@ZackaryH8 ZackaryH8 self-assigned this Feb 4, 2022
@ZackaryH8
Copy link
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
Copy link
Contributor Author

qwrwed commented Feb 11, 2022

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
Copy link
Contributor Author

qwrwed commented Feb 12, 2022

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
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants