-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improved type definitions for JSON api responses #33037
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
Comments
This is exactly why I'm against this proposal |
That would be ideal, but it doesn't seem possible to solve with Typescript alone as they don't 'Add or rely on run-time type information in programs, or emit different code based on the results of the type system.' This proposal seems more about trying to improve the type definitions for the As far as I know, the primitive types which json doesn't support are
or filtering them out from the resulting type. I think this proposal would be especially helpful as it's an incorrect type when fetching data, where we already don't have much type information. Helping to prevent people from having runtime errors by calling date functions on a string seems extremely worthwhile. |
Counter proposal, |
I think there are two points emerging here that are worth picking apart.
Obviously you seem very concerned about point 1, and rightly so it's a source of many mistakes. However, this proposal is really about addressing point 2. I put the following caveat:
in because I wanted to make it clear that I was not trying to address point 1 with this proposal. You are right that the data is |
Use a type validation/data mapping library like,
Adding With a data mapping library, it won't even really matter what the actual response is. You don't have to restrict yourself to just "valid JSON types" This function dangerouslyPretendIsJsonType<T extends JSONType> (mixed : unknown) : T {
return mixed as T;
}
/*snip*/
.then((response) => {
return response.json().then(json => dangerouslyPretendIsJsonType<MyJsonType>(json));
})
.then((json) => {
//Do stuff with strongly typed, but dangerously casted, `json`
}) |
As noted, there are some very nice libraries available these days for enforcing (yay!) the shape of JSON data off the wire, as well as some options for generating In terms of adding this definition to the library, we think this kind of definition best belongs in user space as people tend to have a lot of different opinions about what constitutes valid and what doesn't. On balance the overall gained value is IMO not that great since the restrictions around which types are JSONable and which aren't isn't that hard to internalize. |
@RyanCavanaugh thanks for your response, and all the great work that you do on Typescript! It seems like there are two scenarios:
I actually agree with @AnyhowStep's proposal to change the type to Given these users doing an implicit cast already, it seems like it would benefit them to show where the type they are trying to cast to is impossible, given the json spec. It seems like the original mapped type was too opinionated as to date serialisation. If it was instead:
this would be agnostic of how they serialise their Dates, but also show them that the type they are expecting is impossible.
I agree that the restrictions aren't hard to internalise (if you know they exist), but I also doubt that developers always check their domain models for any dates before casting. |
@richthornton I agree with your points and that whilst it's not hard to internalise the JSON spec, one of the main advantages of a type system is that it puts knowledge back in the world rather than requiring it to be in the developer's head. Even small amounts of individual knowledge eventually accumulate over the size of an entire program. From the JSON spec do you not think the following definition would be more correct?
As we know that JSON can never represent anything except these primitive types, or arrays and objects of them. I respect the original decision to close this issue as I understand the conservative viewpoint of the TypeScript team, but I just wanted to include this for posterity. |
Search Terms
fetch body json generic
Suggestion
There are actually two suggestions here that would work together. I'll present them separately.
A generic version of the
json()
method on a fetchResponse
with the following signature:json<T>(): Promise<T>
.A
Json<T>
type that is equivalent toT
only ifT
contains properties that can be represented as valid JSON. Implementation below. The main thing here is to enforce that any properties of typeDate
inT
are correctly mapped from astring
in order to convertJson<T>
toT
.Use Cases
The use cases for both features are pretty much the same so I'll deal with them together.
When fetching data from an API we create types to represent the data that is being returned. For example, say we have the following type:
And we want to fetch one from an API then we'll write a function like so:
The problem here is that elsewhere in the application if we try use the
date
property on aFoo
then it won't actually be of typeDate
, but most likely will be astring
because that is how it is often represented in JSON. This leads to runtime errors, usually along the lines of some Date method being undefined for string.Obviously we could put this down to bad programming and recognise that we should create some type to represent the data that's actually being returned from the API, for example:
and then update the API function to be:
However, it places a large burden on the developer when this needs to be done for many types and it is still error prone if the developer forgets to create the DTO type.
In our projects we have found a better way to mitigate this problem by defining a
Json<T>
type like so:We can then define a function
get<T>
along these lines:And so our implementation of
getFoo
becomes:Basically, the upshot of having the type
Json<T>
is that it is now not possible to forget to supply the mapping fromJson<Foo>
toFoo
because in this case aJson<Foo>
is not directly assignable toFoo
due to theJson<Foo>
knowing that thedate
property is actually astring
in the Json payload. Furthermore, there is no longer a need to mandate that a DTO type be created for every object (or every object that contains aDate
property). This leads to code that is therefore less prone to errors.From our perspective it would therefore seem to make sense if something along these lines was incorporated into lib.d.ts. Our suggestion is therefore to add the
Json<T>
type definition and update thejson()
method to have the signaturejson<T>(): Promise<Json<T>>
.However, we obviously realise that there might be some reservations with this implementation. The main points that spring to mind are:
string
in a JSON payload? This certainly seems like the most common representation, but I'm sure there will be others out in the wild.Date
that suffer from this problem?To give my two cents on this, I think that the solution proposed does not prohibit someone from just calling
json<any>()
which would effectively give them the existing behaviour, and in fact for backwards compatibility it would probably make sense to default the generic argument toany
so that existing code of that didn't specify the generic argument would still be valid. Essentially, I think that this proposal enhances the types for most cases without regressing things in the other cases.Anyway, I hope this can at least form the start of something useful that can be included in lib.d.ts to improve the typing of data returned from an API.
Checklist
My suggestion meets these guidelines:
The text was updated successfully, but these errors were encountered: