Skip to content
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

libdefs: Fix/suppress most glossed-over errors in react-intl libdef #5308

Merged
merged 16 commits into from
Mar 25, 2022

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Mar 23, 2022

Like we did for React Nav's libdefs in #5263. This will help us toward the RN v0.65 upgrade, #5230, by preparing to upgrade Flow.

Done by temporarily upgrading to Flow v0.149, which has this bugfix from Flow v0.143—

  • Previously, errors in library files were sometimes being missed due to a bug. This has been fixed, which may expose errors in library files that were not previously being reported.

—and going through the newly caught errors.

@gnprice, three of these errors remain after this branch. Would you be up for taking a stab at the best approach for them? You could add some commits if you see something easy, or else let me know a good direction and I'll do it. Thanks!

Error ---------------------------------------------------------------------------- flow-typed/react-intl_vx.x.x.js:64:36

Cannot resolve name `Pick`. [cannot-resolve-name]

   64|   declare var DEFAULT_INTL_CONFIG: Pick<
                                          ^^^^


Error --------------------------------------------------------------------------- flow-typed/react-intl_vx.x.x.js:173:43

Cannot resolve name `Exclude`. [cannot-resolve-name]

   173|   declare export type FormatDateOptions = Exclude<DateTimeFormatOptions, 'localeMatcher'> &
                                                  ^^^^^^^


Error --------------------------------------------------------------------------- flow-typed/react-intl_vx.x.x.js:780:37

Cannot resolve name `Omit`. [cannot-resolve-name]

   780|   declare type OptionalIntlConfig = Omit<IntlConfig, $Keys<typeof DEFAULT_INTL_CONFIG>> &
                                            ^^^^

Looks like they're uses of TypeScript's utility types with those names. (This libdef began from a TypeScript to Flow translation using Flowgen.) I am kind of puzzled by the uses of Exclude and I wonder if they really meant to be uses of Omit

@chrisbobbe chrisbobbe requested a review from gnprice March 23, 2022 22:34
@chrisbobbe chrisbobbe changed the title libdefs: Fix most glossed-over errors in react-intl libdef libdefs: Fix/suppress most glossed-over errors in react-intl libdef Mar 23, 2022
@gnprice
Copy link
Member

gnprice commented Mar 24, 2022

Thanks! These changes all look good -- please merge at will.

I'm looking at those three remaining errors.

@gnprice
Copy link
Member

gnprice commented Mar 24, 2022

For Pick, it looks like it's basically doing the same job as the SubsetProperties in our generics.js.

Sadly I'm not finding a way to implement a version with the same interface as Pick, where it takes a union of the keys. I hoped this might work:

type Pick<T, Keys> = SubsetProperties<T, { [Keys]: mixed }>;

but it doesn't (try-flow). And in retrospect that makes sense -- I think the issue is probably that $ObjMapi on an indexer like { ['a' | 'b']: mixed } is going to produce a type with a corresponding indexer, { ['a' | 'b']: /* something */ }, not an object type with separate property entries for a and b.

So the best available solution may instead be to copy in SubsetProperties, like declare type SubsetProperties…, and then edit each of the Pick<…> sites to use that instead.

@gnprice
Copy link
Member

gnprice commented Mar 24, 2022

For Omit, I'm afraid it looks like the same story. Here I was more hopeful because this definition seems like it really should work:

type Omit<T, Keys> = $Rest<T, {| [Keys]: mixed |}>;

But it seems to become another victim of indexers just being kind of broken:

type Omit<T, Keys> = $Rest<T, {| [Keys]: mixed |}>;

type T = {| a: number, b: string |};

(x: Omit<T, 'b'>): {| a: empty |} => x; // error, good
(x: Omit<T, 'b'>): {| a: number |} => x; // BAD error

// What happens instead:
(x: Omit<T, 'b'>): {| a: number | void, b: string | void |} => x; // weird!
// or spelling that out:
(x: $Rest<T, {| ['b']: mixed |}>): {| a: number | void, b: string | void |} => x; // weird!

// It's fine if there's normal properties instead of an indexer:
(x: $Rest<T, {| b: mixed |}>): {| a: number |} => x;

(try-flow)

So there also, I think the best solution may be to edit each of the sites, here to use $Rest instead.


For one of them:

  declare type OptionalIntlConfig = Omit<IntlConfig, $Keys<typeof DEFAULT_INTL_CONFIG>> &
    $Rest<typeof DEFAULT_INTL_CONFIG, { ... }>;

I think one can also simplify by taking advantage of Flow's spreads to make the Omit unnecessary:

  declare type OptionalIntlConfig = { ...IntlConfig, ...$Rest<typeof DEFAULT_INTL_CONFIG, { ... }> };

@gnprice
Copy link
Member

gnprice commented Mar 24, 2022

And then for Exclude, I agree that all of its uses appear to be mistakes where Omit was intended. (That can fairly be blamed on the TypeScript folks, perhaps -- those two names are basically synonyms and it's not at all clear how to say which one goes with which meaning.)

…s type

This isn't meant to refer to the type of a value on the Intl object,
which the dot suggests. There isn't a value at this key:
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L8-L15

For this type's definition, which uses the dollar sign, see
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L100-L115
This isn't meant to refer to the type of a value on the Intl object,
which the dot suggests. There isn't a value at this key:
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L8-L15

For this type's definition, which uses the dollar sign, see
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L187-L196
…type

This isn't meant to refer to the type of a value on the Intl object,
which the dot suggests. There isn't a value at this key:
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L8-L15

For this type's definition, which uses the dollar sign, see
  https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L148-L160
…format

Flow doesn't seem to have a counterpart to TypeScript's
`Parameters` [1] type; so, copy out the types of the function's
params.

[1] https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype
Flow doesn't seem to have a counterpart to TypeScript's
`Parameters` [1] type; so, find how Flow types the function's param
and copy that:
  https://github.com/facebook/flow/blob/main/lib/intl.js#L170

[1] https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype
Flow doesn't seem to have a counterpart to TypeScript's
`Parameters` [1] type; so, copy out the type of the function param.

[1] https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype
Flow doesn't seem to have a counterpart to TypeScript's
`Parameters` [1] type; so, copy out the type of the function param.

[1] https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype
…eturn

Here's Flow's built-in definition for the `Intl` object [1]:

  declare var Intl: {
    Collator: Class<Intl$Collator>,
    DateTimeFormat: Class<Intl$DateTimeFormat>,
    NumberFormat: Class<Intl$NumberFormat>,
    PluralRules: ?Class<Intl$PluralRules>,
    getCanonicalLocales?: (locales?: Intl$Locales) => Intl$Locale[],
    ...
  }

(It uses Flow's built-in `Class` type:
https://flow.org/en/docs/types/utilities/#toc-class )

So, for Intl, the names with $ stand for instance types, and the
names with . stand for class types.

It looks like the touched lines of code really want to say something
like

   getDateTimeFormat: ConstructorOf<Intl.DateTimeFormat>

, where `ConstructorOf` is a name I just made up, and Flow doesn't
yet have a tool that would fit that name; see
  facebook/flow#1409 (comment)

Without that, it's at least easy to preserve the return value of
each of these functions. As with any constructor function, the
return value is an instance object, not the class object, which is
why we make this change.

[1] https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L8-L15
Well, sort of; see the comment, repeated at each site.

It'd be better if Flow gave an error at each of these sites, which
we could then suppress. Then when the Flow issue is fixed upstream,
we'd get an alert that the suppressions were no longer necessary,
and adapting would be as simple as removing the suppressions.
This fixes one of the things Flow glosses over and would catch when
we migrate this to a .js.flow file, with an error like this:

  Cannot use `React.Element` [1] without 1 type argument.
    [missing-type-arg]

Without having a particular type in mind to pass, just use the
highest, most general type allowed. See definition [1]:

  /**
   * Type of a React element. React elements are commonly created using JSX
   * literals, which desugar to React.createElement calls (see below).
   */
  declare type React$Element<+ElementType: React$ElementType> = {|
    +type: ElementType,
    +props: React$ElementProps<ElementType>,
    +key: React$Key | null,
    +ref: any,
  |};

[1] https://github.com/facebook/flow/blob/v0.141.0/lib/react.js#L174-L183
This doesn't agree with the definition on React$Component [1]:

  static displayName?: ?string;

[1] https://github.com/facebook/flow/blob/v0.141.0/lib/react.js#L87
This fixes one of the things Flow glosses over and would catch when
we migrate this to a .js.flow file, with an error like this:

  Error -------------------- flow-typed/react-intl_vx.x.x.js:353:36

  Cannot use `PluralRules` as a type. A name can be used as a type
  only if it refers to a type, interface, class, or enum definition.
  To get the type of a non-class value, use `typeof`.
  [value-as-type]

     353|     pluralRules: {| [key: string]: Intl.PluralRules |};
                                             ^^^^^^^^^^^^^^^^

I think this is what's going on:

Flow treats Intl.Foo as a value, because Intl is a `declare var`,
not a `declare type` [1]:

  declare var Intl: {
    Collator: Class<Intl$Collator>,
    DateTimeFormat: Class<Intl$DateTimeFormat>,
    NumberFormat: Class<Intl$NumberFormat>,
    PluralRules: ?Class<Intl$PluralRules>,
    getCanonicalLocales?: (locales?: Intl$Locales) => Intl$Locale[],
    ...
  }

I think that means that if we want to use the declared type of an
Intl.Foo, we'd normally want to use `typeof Intl.Foo`.

But most of the actual Intl members with `Class<…>` are special,
because a class value can be used as a type without `typeof`.

I think Intl.PluralRules doesn't get that special treatment, because
it's got `?Class<…>` -- which expands to the union type
`Class<…> | null | void`. Union types aren't class definitions, so
the special treatment doesn't apply and we should use `typeof`.

[1] https://github.com/facebook/flow/blob/v0.141.0/lib/intl.js#L8-L15
@chrisbobbe chrisbobbe force-pushed the pr-fix-react-intl-libdef branch from 8b1d497 to 2bb39a3 Compare March 25, 2022 18:43
@chrisbobbe chrisbobbe merged commit 2bb39a3 into zulip:main Mar 25, 2022
@chrisbobbe
Copy link
Contributor Author

Thanks! These changes all look good -- please merge at will.

I'm looking at those three remaining errors.

Great, thanks! Merged, and I'll address the three remaining ones in a new PR.

@chrisbobbe chrisbobbe deleted the pr-fix-react-intl-libdef branch March 25, 2022 18:44
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 25, 2022
This doesn't actually improve Flow coverage or change any types;
both Omit and Exclude are utility types in TypeScript that Flow
doesn't have. But in the original TypeScript, the use of Exclude for
these was a mistake; they should have been Omit; see discussion at
  zulip#5308 (comment)

We're about to go through all uses of Omit and replace them with
something that works; these will then be fixed.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 25, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 25, 2022
…lity

As suggested by Greg:
  zulip#5308 (comment)

This definition is copied from our src/generics.js. When we move
this libdef to a .js.flow file, we'll deduplicate the definition by
importing it from src/generics.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 25, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 1, 2022
This doesn't actually improve Flow coverage or change any types;
both Omit and Exclude are utility types in TypeScript that Flow
doesn't have. But in the original TypeScript, the use of Exclude for
these was a mistake; they should have been Omit; see discussion at
  zulip#5308 (comment)

We're about to go through all uses of Omit and replace them with
something that works; these will then be fixed.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 1, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 1, 2022
…lity

As suggested by Greg:
  zulip#5308 (comment)

This definition is copied from our src/generics.js. When we move
this libdef to a .js.flow file, we'll deduplicate the definition by
importing it from src/generics.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants