Fix flow errors in redux-query updates #2051
Conversation
ChristopherChudzicki
force-pushed
the
jw/update-react-redux-and-dependencies-cc-edits
branch
from
b762872 to
0c6695c
Compare
There was a problem hiding this comment.
I left comments explaining and resolving the Flow errors.
I will say, overall, I found these issues annoying not helpful, and in contrast, Typescript handles al these issues better. Admittedly, our version of Flow is 5 years out of date. But I believe these are philosophical differences between TS and Flow, not things that have improved.
Another option, rather than using the resolutions I've provided, would be to just add redux-query to the [untyped] section of our Flowconfig, which would make redux-query be treated as untyped.
untyped doesn't make me feel good but:
- That's the current status on
main - Many of these warnings seem annoying
So if Flow isn't being helpful in regards to this library, it worth considering, IMO.
| import type { Response } from "redux-query" | ||
| import type { HttpResponse } from "../../../flow/httpTypes" |
There was a problem hiding this comment.
This was giving the error
Cannot import Response because there is no Response export in redux-query.
Note: import type { Response } from "redux-query" exists on main branch.
Error explanation
Why doesn't mport type { Response } from redux-query error on main branch?
Because, by default, Flow doesn't report non-existent type imports for untyped modules, and our version of redux-query on main is untyped.
redux-queryadded Flow support in version 3.0.0; main branch uses 2.3.1- So we are importing a non-existent type on
mainbranch, too. It's treated asany(implicit)by Flow because apparently Flow treats non-existent imports asany(implicit)when the import target is untyped. And it is untyped onmainbranch, but it is no longer untyped on this branch, so now Flow complains about the non-existing import.- Treating non-existent type imports as
any(implicit)seems like poor design to me. (It's reasonable for untyped classes/functions/etc to ease adoption). This behavior can be turned off with strict mode, but we're not using strict mode.
- Treating non-existent type imports as
Resolution
We need to stop importing the non-existent type. We could:
- Replace our usage of
Responsewithany.- IMO, this would be OK. It provides no type safety or documentation hinting, but we weren't getting type safety on
maineither, since Response was being treated asany(implicit)anyway.
- IMO, this would be OK. It provides no type safety or documentation hinting, but we weren't getting type safety on
- Replace with an appropriate type.
I tried to do (2): Based on usage, it looks like everywhere we were using Response we just expected the object to have structure { body: <SomeDataType> }. The HttpResponse has structure { body, status }, so that seems to fit.
Request: Double check that the objects returned by editProfile and getCurrentUser actually fit HttpResponse (have body and status). I don't have mitxonline running so I couldn't check this.
There was a problem hiding this comment.
After much futzing with my own installation, was able to confirm. (registration/login isn't something I usually care about since i just work in the front end app, but being able to tell that the registration works requires the connection to edx to actually... work.
| const mapPropsToConfig = ({ qsParams }) => | ||
| mutateAsync(queries.auth.registerConfirmEmailMutation(qsParams)) | ||
| const mapPropsToConfig = ({ qsParams }) => { | ||
| const { type, ...config } = mutateAsync( | ||
| queries.auth.registerConfirmEmailMutation(qsParams) | ||
| ) | ||
| return config | ||
| } |
There was a problem hiding this comment.
This was giving an error:
Cannot call connectRequest with mapPropsToConfig bound to mapPropsToConfigs
because property type is missing in QueryConfig [1] but exists in
MutateAsyncAction [2] in the return value.
Again, Flow did not complain on main because redux-query is untyped on main, so anything is allowed.
The error occurs because:
- Now
redux-queryhas Flow types... redux-queryexpectsmapPropsToConfigto return a QueryConfig, which their type definition defines as an exact type, so it is not allowed to have extra keys. But the return value ofmutateAsyncdoes have an extra key, namely"type".- Javascript doesn't care about the extra properties, which is why everything seems to work fine.
- We use Flow 0.95, in which exact types are denoted with pipes
type Exact = {| ... |}. Exact types are now the default in Flow. - In contrast, Typescript does not have the concept of exact types. So in Typescript, a declaration like
is perfectly fine. (TS does actually demand no extra properties when assigning an object literal to a type, a pragmatic, extra-check they call excess property checking.type Dog = { name: string } const sayWoof = (dog: Dog) => console.log(`${dog.name): woof woof`) const leo = { name: "Leo", owner: "Chris" } sayWoof(leo) // TS won't complain about extra "owner" property, but Flow will.
IMO Flow's enthusiasm for exact types is paranoid and unhelpful. TS's behavior is more natural, IMO. For reference: microsoft/TypeScript#12936 (comment)
Resolution
I resolved the issue by bending to Flow's demand 🤷. There's probably also a way to typecast it to QueryConfig, or suppress the error with a $FlowFixMe.
There was a problem hiding this comment.
Basically all i got from this one was TS>Flow
There was a problem hiding this comment.
(kidding - makes sense, although it doesn't make sense. If that... makes sense)
| transform: (auth: AuthResponse) => ({ auth }), | ||
| transform: (auth: ?AuthResponse) => ({ auth }), |
There was a problem hiding this comment.
There was a Flow error:
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/containers/pages/register/RegisterDetailsPage.js:184:5
Cannot call mutateAsync with auth.registerDetailsMutation(...) bound to the
first parameter because AuthResponse [1] is incompatible with null or
undefined [2] in the first argument of property transform.
This error is occurring because:
- Now
redux-queryhas flow types... - And their flow type definition for
Transformiswhich saystype Transform = (body: ?ResponseBody, text: ?ResponseText) => { [key: string]: any };
bodymight be undefined or null. - ...whereas we were saying it's not allowed to be.
Resolution
If body ever actually is undefined/null, I suspect we will have issues at runtime. Currently, the transformed object would be { auth: undefined } in that situation. (Maybe that's fine actually. Might just be treated as unauthenticated?)
Anyway, this doesn't change the runtime behavior, just makes the signature match what redux-query's flow definitions are expecting.
There was a problem hiding this comment.
Yes, to confirm - that's no logged-in user which is mutated to a not-logged in user, but auth stays undefined because they're not authed
| @@ -8,7 +8,8 @@ | |||
| "rules": { | |||
| "no-unused-vars": [ | |||
| "error", { | |||
| "argsIgnorePattern": "^__" | |||
| "argsIgnorePattern": "^__", | |||
| "ignoreRestSiblings": true | |||
There was a problem hiding this comment.
This means that in const { a, ...others } = thing, a won't be treated as unused, even if it is never used again. This allows object spread ... to be used for omitting properties from an object without triggering errors. https://eslint.org/docs/latest/rules/no-unused-vars#ignorerestsiblings
|
@ChristopherChudzicki THANK YOU. I'm going to merge this in and double check things before I fix those tests that are hanging (if I haven't already, TBH at this point IDK any more) |
JenniWhitman
merged commit 277aa9c
into
jw/update-react-redux-and-dependencies
…#2030) * Redid the upgrade on a new branch because I had weird dependency issues. * before jest-codemods * update for lifecycle methods mounting * test updates * mock useContext for shallow rendered tests * change to shallow helper with chris's mock * putting away to test pr * added comment to top of scratch file * put main containers back on deep render * notification container back to deep render * remove extra console.log statements and addd in second describe statement in CourseProductDetailEnroll_test.js to facilitate splitting tests per render method * moved all tests that were unable to make calls due to shallow render over to a separate block which calls deeprender only. Have to put changes back in to fix teh generated course data and should be set. * fix some linting issues * fix some linting issues * I totally forgot to reinstall pre-commit after I reinstalled linux on this. fmt * fix flow * move Response location * move flow up to root * response == queryresponse? * updated components with links. need actual links. * fix flow errors (#2051) * test updates to passing - moved a lot of tests to just test the behavior, shallowly, ignoring any calls. we otherwise test that these components are working with redux (which is what makes those calls that were previously asserted by sinon. Now I'm taking those as fact since we want to test the behavior of this component with the information provided, this is a shallow render, ignoring those calls (and un-complicating the test) * format * unused imports * Revert "updated components with links. need actual links." This reverts commit fd4163f. * fixes for poor merge on the tests - and related formatting * PR response fixes * fix fmt --------- Co-authored-by: Chris Chudzicki <[email protected]>
What are the relevant tickets?
Description (What does it do?)
Screenshots (if appropriate):
How can this be tested?
Additional Context