-
Notifications
You must be signed in to change notification settings - Fork 2
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
Comments
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 |
Swagger file, for reference Couple of questions:
re: compiling my app, is there anything in particular giving you trouble? |
I don't remember the exact name but there is a TypeScript tool
I assume it's when the types are actually generated
No problems now, all sorted! |
Looks like this might have been the tool: https://www.npmjs.com/package/openapi-typescript |
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). |
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? |
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 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. |
I think adding This should work as a solution, though it will require some refactoring - I've written a couple of quick scripts - one to add the requirements ( Edit: I've also added a script to create a JSON map (see here) of the new type names (only those that are named |
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:
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 typestring | 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:
to
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.
The text was updated successfully, but these errors were encountered: