Page MenuHomePhabricator

[keyserver/lib] Improve `tcomb` types
ClosedPublic

Authored by michal on Apr 17 2023, 7:12 AM.
Tags
None
Referenced Files
F3347731: D7467.diff
Fri, Nov 22, 1:07 PM
Unknown Object (File)
Tue, Nov 5, 4:30 AM
Unknown Object (File)
Tue, Nov 5, 4:30 AM
Unknown Object (File)
Tue, Nov 5, 4:29 AM
Unknown Object (File)
Tue, Nov 5, 4:29 AM
Unknown Object (File)
Tue, Nov 5, 4:29 AM
Unknown Object (File)
Tue, Nov 5, 4:29 AM
Unknown Object (File)
Mon, Nov 4, 1:25 AM
Subscribers

Details

Summary

These changes should improve:

  • Better union validators, by making the generics covariant (+). Previously flow would fail at t.union([t.String, t.Bool]) but now types it as TUnion<string | boolean>
  • Making TInterface, TStructProps and tShape generic, and now tShape<{a: boolean, b: string}>({a: t.Bool, b: t.String}) will be typechecked (both the type argument and validator argument have to match)
  • Instead of TType<T> being at the top of an inheritence hierarchy, it's now a union of the possible validator types. Additionaly meta.kind has now specific values for each validator type (taken from tComb docs). This allows us to check the type of the validator with meta.kind and type-safely get e.g. meta.type when meta.kind === 'maybe'

The last change also required a slight change of meaning of the generic T for some validators. For example previously: TList<T> = TType<Array<T>>, and now TList<Array<T>> = TType<Array<T>>. This is also clearer (at least to me): now for every validator variant the generic type represents the whole type that is validated. This theoretically allows for nonsesical types like TList<NonArrayType> but this can't happen if we are using the t.list, t.dict etc. functions.

Test Plan

Run yarn flow and check for errors

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/responders/entry-responders.js
43 ↗(On Diff #25219)

These are typed as any because they don't correspond to any named flow type.

ashoat requested changes to this revision.Apr 17 2023, 10:26 AM
ashoat added inline comments.
keyserver/src/responders/entry-responders.js
43 ↗(On Diff #25219)

We should avoid any – it completely breaks any type it touches. I think what you are doing is equivalent to typing the whole entryQueryInputValidator as any, and anything entryQueryInputValidator touches will basically be turned to any.

any is very, very bad. We should always try to find alternatives. Your explanation is very short and does not give any context on what else you tried, and why you made this decision ("doesn't correspond to any named Flow type" seems to imply you are skipping typing something).

Did you mean to use mixed here? Are you skipping typing something that you should type?

This revision now requires changes to proceed.Apr 17 2023, 10:26 AM

Improve typings for entry query validators

ashoat requested changes to this revision.Apr 21 2023, 6:45 AM

Great work!! This is significantly improved :)

keyserver/src/responders/entry-responders.js
46–52 ↗(On Diff #25508)

Thanks for typing this!! Some changes I'd like to suggest

lib/utils/validation-utils.js
76–90 ↗(On Diff #25508)

These ones are still using any. I think you can type them with MediaMessageServerDBContent, you'll just need to split into two types and export both. Would you prefer to do that here or in a follow-up diff?

This revision now requires changes to proceed.Apr 21 2023, 6:45 AM

Thanks, I should've caught these! Fixed the added type, removed any usage.

This revision is now accepted and ready to land.Apr 21 2023, 7:57 AM
lib/flow-typed/npm/tcomb_v3.x.x.js
29

Spacing is off here (and in all other examples below) – please make sure it's consistently two-space indents

97

Why do we use the {| |} here but not elsewhere?

{ } default behavior is ambiguous, so in most libdefs that get contributed upstream, it's good to disambiguate by using the more specific {| |} or { ... } variants

If we're not contributing upstream we can stick with { }, but in that case it would be good to be consistent