-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Use more specific JSON types in TypeScript #82
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good improvement. 👍
We can deal with the enforcement of no body
for GET
s separately. I don't really have strong feelings about doing that anyway (it's technically allowed by the spec FYI, but highly discouraged, very much an anti-pattern, and of no practical value).
@@ -1,3 +1,9 @@ | |||
type JSONObject = {[key: string]: JSONValue}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this does what you intend it to. The way this is defined, no object is considered "JSON stringifyable" unless it has an index signature. For example, this would fail:
interface Foo { foo: number }
const foo: Foo = { foo: 123 }
const json: JSONObject = foo // Error: Foo is not assignable to JSONObject because it has no index signature
I don't think there is currently a way to express this constraint correctly.
Besides, JSON.stringify() actually doesn't care if an object has methods. It will simply omit them (unlike postMessage()
, which throws an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the response though, not for being stringifyiable, for stringification we're quite lax:
Line 5 in b6ed9df
export type JSONStringifyable = string | number | boolean | null | object; |
Hopefully a JSON type will be included in TypeScript at some point: microsoft/TypeScript#1897 For now, we just define it manually.