remove type param from axios such that responses and requests are all now mixed #2464
Conversation
|
It seems to me that setting the return types as axios.get('/foo').then(res => { console.log(res.foo) })does that throw an error? Can you write a test and make sure that's possible? |
gantoine
added
enhancement
|
@AndrewSouthpaw being pedantic here: The response itself does not have Perhaps you meant: axios.get('/foo').then(res => { console.log(res.data.foo) })In this case it will fail to type check as well (rightfully so). You aren't strictly required to cast to type Foo = { foo: string }
(res: $AxiosXHR<Foo>) => console.log(res.data.foo.toUpperCase()type Foo = { foo: string }
(res) => console.log(((res.data: any): Foo).foo.toUpperCase()In both cases, you're saying "Flow just trust me on this one" and we all know what kind of lie that is to Flow and ourselves ;) If we treat |
|
RE: Thanks for expanding on your thoughts! I hear them, and they make sense. For me, at most, as a consumer of axios I'd want to do something like type Foo = { foo: string }
(res: $AxiosXHR<Foo>) => console.log(res.data.foo.toUpperCase()If I had to do (res: $AxiosXHR<Foo>) => {
if (res.data.foo) {
...
}
}or anything else like it using a validation library, I would get very, very grumpy. Yes, it's not technically type safe, and also I'm okay with that; it's something I like about Javascript, that's it's not actually a typed language and sometimes I can choose to not worry about it without having to validate everything. If I'm actually concerned about it I can pass it through the validation, but generally I'd rather not worry about it. I think that's a reflection of how I like to code and what purpose I want Flow to serve for me. Everyone is different. So, my vote is to not merge this PR. I'm also happy to be outvoted and just keep my own older version locally. @gantoine @villesau @adityavohra7. Feel free to CC other people e.g. maintainers of this libdef if you want. |
… and requests are all now mixed
LoganBarnett
force-pushed
the
2461-axios-use-mixed-payloads
branch
from
25cd0eb
to
b4503d4
Compare
|
@AndrewSouthpaw perhaps the problem here is that we don't have a great way to do a forceful cast? At risk of being obvious, I'm in the camp of wanting more "soundness" which necessitates more strictness. That said, I can continue to use As an aside, the CI fails with |
|
Man, what I'd do for a forceful recast that wasn't Yeah curious to hear input from others. RE: aside: The CI tests are ... kinda crap. Restarted. |
|
I'm with @LoganBarnett here. Soundness should be priority, and that's the reason we even use tools like Flow :) Although, I totally understand @AndrewSouthpaw s point, and checks would probably make annoy me too. But I have news for you: Flow have supported optional chaining now for a while which makes this kind of checks a lot easier :) So, I would go with |
|
@villesau A couple thoughts about optional chaining... it's only stage 1 right now, so offering it as a solution doesn't seem fair to the community. The bigger issue in my opinion is that it only solves part of the problem. My example wasn't complete. Here's what I'm actually afraid of needing to do: type Foo = { a: string, b: number, c: SomeOtherComplexType }
axios.get('/foo').then((res: $AxiosXHR<mixed>) => {
if (typeof res.data.a === 'string' && typeof res.data.b === 'number' /* && argh wtf c */) {
...
}
})Not only is that annoying to write out (or maintain a bunch of type-checking helpers), basically my fear is that we'd be forcing developers to perform a lot of runtime checks in order to provide type safety. It's potentially (probably?) totally unfounded, but I'm having concerns about performance impact on slow phones, and also the crummy dev experience of just having to actually perform type checking on every blasted AJAX request in my codebase. I'm also definitely willing to be overruled here in the face of a majority vote. I'd love to get some other opinions to weigh in, and I can shamefully override my |
Well.. JSX is not part of Javascript standard and neither is Flow it self so you will always need a transpiler for your Flow code. And flow clearly seems to go optional chaining path as they have implemented the support for it. At least they have strong belief in optional chaining, I don't think that kind of features are implemented just for fun. There are only very few experimental Javascript features Flow supports optional chaining being one of them. But your example indeed looks very annoying. Didn't think of more complex objects in previous message. Do we have any good examples of other similar cases? |
Fair point. I think we draw the line in different places, and I do see what you're saying. As far as complex cases, I could point to any number of them within our codebase. We have plenty server calls and they often return complex objects of different types. Did you want actual examples, or something else? |
|
I was thinking about examples of similar libraries and their libdefs :) |
|
I have an example with a relatively flat structure: export type CellFields = {
frontImage: string,
rightImage: string,
bottomImage: string,
topImage: string,
backImage: string,
leftImage: string,
}
export type Cell = {
fieldType: 'cell',
_id: DbId,
fields: CellFields,
name: string,
definitionId: DbId,
fields: {
frontImage: string,
topImage: string,
rightImage: string,
bottomImage: string,
backImage: string,
leftImage: string,
},
}This is one of my simpler ones. The validation code for it is downright monstrous at ~240 lines: export default (json: mixed): Cell | Error => {
if (json === null) {
return new Error('Could not deserialize json because the value is null.')
} else if (typeof json == 'undefined') {
return new Error(
'Could not deserialize json because the value is undefined.',
)
} else if (json instanceof Error || typeof json != 'object') {
return new Error('Could not deserialize object "' + String(json) + '"')
} else {
const name = deField('name', deString, json.name)
if (name instanceof Error) {
const error: Error = name
return new Error('Could not deserialize field "name": ' + error.message)
} else {
const fieldType = deField(
'fieldType',
(x: mixed) => {
if (typeof x != 'string') {
return new Error(
'Could not deserialize "' + String(x) + '" into a string.',
)
} else if (x === 'cell') {
return x
} else {
return new Error(
'Could not deserialize "' +
String(x) +
'" into a string with the value "cell".',
)
}
},
json.fieldType,
)
if (fieldType instanceof Error) {
const error: Error = fieldType
return new Error(
'Could not deserialize field "fieldType": ' + error.message,
)
} else {
const _id = deField(
'_id',
(v: mixed | Error) => {
const either = deString(v)
if (either instanceof Error) {
return either
} else if (typeof either == 'string') {
if (either.match(/^[a-f0-9]+$/)) {
return (either: DbId)
} else {
return new Error(
'Could not deserialize "' +
String(v) +
'" into a valid database ID.',
)
}
} else {
return new Error(
'Could not deserialize "' +
String(v) +
'" into a valid database ID.',
)
}
},
json._id,
)
if (_id instanceof Error) {
const error: Error = _id
return new Error(
'Could not deserialize field "_id": ' + error.message,
)
} else {
const definitionId = deField(
'definitionId',
(v: mixed | Error) => {
const either = deString(v)
if (either instanceof Error) {
return either
} else if (typeof either == 'string') {
if (either.match(/^[a-f0-9]+$/)) {
return (either: DbId)
} else {
return new Error(
'Could not deserialize "' +
String(v) +
'" into a valid database ID.',
)
}
} else {
return new Error(
'Could not deserialize "' +
String(v) +
'" into a valid database ID.',
)
}
},
json.definitionId,
)
if (definitionId instanceof Error) {
const error: Error = definitionId
return new Error(
'Could not deserialize field "definitionId": ' + error.message,
)
} else {
const fields = deField(
'fields',
(json: mixed): CellFields | Error => {
if (json === null) {
return new Error(
'Could not deserialize json because the value is null.',
)
} else if (typeof json == 'undefined') {
return new Error(
'Could not deserialize json because the value is undefined.',
)
} else if (json instanceof Error || typeof json != 'object') {
return new Error(
'Could not deserialize object "' + String(json) + '"',
)
} else {
const frontImage = deField(
'frontImage',
deString,
json.frontImage,
)
if (frontImage instanceof Error) {
const error: Error = frontImage
return new Error(
'Could not deserialize field "frontImage": ' +
error.message,
)
} else {
const topImage = deField(
'topImage',
deString,
json.topImage,
)
if (topImage instanceof Error) {
const error: Error = topImage
return new Error(
'Could not deserialize field "topImage": ' +
error.message,
)
} else {
const rightImage = deField(
'rightImage',
deString,
json.rightImage,
)
if (rightImage instanceof Error) {
const error: Error = rightImage
return new Error(
'Could not deserialize field "rightImage": ' +
error.message,
)
} else {
const bottomImage = deField(
'bottomImage',
deString,
json.bottomImage,
)
if (bottomImage instanceof Error) {
const error: Error = bottomImage
return new Error(
'Could not deserialize field "bottomImage": ' +
error.message,
)
} else {
const backImage = deField(
'backImage',
deString,
json.backImage,
)
if (backImage instanceof Error) {
const error: Error = backImage
return new Error(
'Could not deserialize field "backImage": ' +
error.message,
)
} else {
const leftImage = deField(
'leftImage',
deString,
json.leftImage,
)
if (leftImage instanceof Error) {
const error: Error = leftImage
return new Error(
'Could not deserialize field "leftImage": ' +
error.message,
)
} else {
const result: CellFields = {
frontImage,
topImage,
rightImage,
bottomImage,
backImage,
leftImage,
}
return result
}
}
}
}
}
}
}
},
json.fields,
)
if (fields instanceof Error) {
const error: Error = fields
return new Error(
'Could not deserialize field "fields": ' + error.message,
)
} else {
const result: Cell = {
name,
fieldType,
_id,
definitionId,
fields,
}
return result
}
}
}
}
}
}
}0.0 I have another one that's closer to 1900 similar lines. I'll spare your scroll wheel for today though ;) This code is all generated for me via a lib. The definition is a lot more lightweight: export default () => degenObject(cellType, [
degenField('name', degenString()),
degenField('fieldType', degenValue('string', 'cell')),
degenField('_id', degenDbId()),
degenField('definitionId', degenDbId()),
degenField('fields', degenObject(cellFieldsType, [
degenField('frontImage', degenFilePath()),
degenField('topImage', degenFilePath()),
degenField('rightImage', degenFilePath()),
degenField('bottomImage', degenFilePath()),
degenField('backImage', degenFilePath()),
degenField('leftImage', degenFilePath()),
])),
])I've done a fair amount of work with I've been using both of these (my work decided they liked While it's easy for me to lean on the soundness argument here, I want to be mindful of biases introduced by my ecosystem. My Flow based projects enforce 100% type coverage, and don't have any unit tests (generally I make database skins for a living, so unit tests tend to only cover what Flow covers already). That means my types need to be solid though since I'm leaning hard on Flow. I'm okay with bringing in a validation/deserialization tool, but I understand that this change to the axios libdef will essentially require such a tool for any non-trivial payload. Or we make a less than pretty FWIW there's no rush here on this PR. Right now we're getting away with a local override :) I think we've made some local adjustments to it as well, which I will try to propagate back in here today. |
😬 As for other libdefs, Ville, I'm not sure. Usually when I've seen libdefs that have to deal with totally ambiguous output, it's |
eslintwas complaining so I had to bypass the check - there's something it doesn't like about the[[call]]syntax, even though this is now preferred to$call. This might be related to #2405. I did not change the[[call]]syntax, so I'm not sure this worked prior.This PR removes the polymorphic types for
axios. This both decouples the request and response payloads (a possible regression of #1975), and improves soundness by moving the validation/deserialization to the consumer (or a blindanycast, which is really what was going on before).Addresses #2461.