Feature Request: Use non-optional properties in types #29
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.
Activity
ZackaryH8 commentedon 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
qwrwed commentedon Jan 31, 2022
Swagger file, for reference
Couple of questions:
re: compiling my app, is there anything in particular giving you trouble?
ZackaryH8 commentedon Feb 1, 2022
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!
qwrwed commentedon 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 commentedon 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.
ZackaryH8 commentedon Feb 4, 2022
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 commentedon Feb 8, 2022
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 commentedon 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 commentedon 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.This should work as a solution, though it will require some refactoring -
Line
'sTfl-3
definition is different fromJourney
'sTfl-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
viaconvert.js
) and one to convert to TS (output/*.json -> ts/*.ts
viatsconvert.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.