Fix flow errors in redux-query updates

[ChristopherChudzicki]

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

• Desktop screenshots
• Mobile width screenshots

How can this be tested?

Additional Context

[ChristopherChudzicki]

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.

[ChristopherChudzicki]

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-query added Flow support in version 3.0.0; main branch uses 2.3.1
• So we are importing a non-existent type on main branch, too. It's treated as any(implicit) by Flow because apparently Flow treats non-existent imports as any(implicit) when the import target is untyped. And it is untyped on main branch, 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.

Resolution

We need to stop importing the non-existent type. We could:

1. Replace our usage of Response with any.
   • IMO, this would be OK. It provides no type safety or documentation hinting, but we weren't getting type safety on main either, since Response was being treated as any(implicit) anyway.
2. 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.

[JenniWhitman]

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.

[ChristopherChudzicki]

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:

1. Now redux-query has Flow types...
2. redux-query expects mapPropsToConfig to 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 of mutateAsync does 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

   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. 

   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.

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.

[JenniWhitman]

Basically all i got from this one was TS>Flow

[JenniWhitman]

(kidding - makes sense, although it doesn't make sense. If that... makes sense)

[ChristopherChudzicki]

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:

1. Now redux-query has flow types...
2. And their flow type definition for Transform is

   type Transform = (body: ?ResponseBody, text: ?ResponseText) => { [key: string]: any };

   which says body might be undefined or null.
3. ...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.

[JenniWhitman]

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

[ChristopherChudzicki]

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

[JenniWhitman]

@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)