Upgrade `react-intl` to a version that uses the new Context API, then to the latest. #4222

chrisbobbe · 2020-08-07T22:13:33Z

No description provided.

gnprice

Thanks @chrisbobbe ! Looks good modulo a few comments.

gnprice · 2020-08-10T23:10:54Z

src/message/messageActionSheet.js

@@ -112,7 +112,7 @@ muteTopic.title = 'Mute topic';
 muteTopic.errorMessage = 'Failed to mute topic';

 const deleteTopic = async ({ auth, message, dispatch, ownEmail, _ }) => {
-  const alertTitle = _("Are you sure you want to delete the topic '{topic}'?", {
+  const alertTitle = _("Are you sure you want to delete the topic ''{topic}''?", {


- Duplicate apostrophes [2] in two messages, since the escape character has been changed from the backslash to the apostrophe. Without this change, "delete the topic '{topic}'" is treated as literal text all the way through. [2] "A pair of ASCII apostrophes always represents one ASCII apostrophe", from the spec linked from the upgrade guide. That spec is at http://userguide.icu-project.org/formatparse/messages

Fun. At a quick grep:

$ rg "'" static/translations/messages_en.json … 219: "Couldn't load information about {fullName}": "Couldn't load information about {fullName}", 220: "What's your status?": "What's your status?",

we have two other messages with a ' in them. Are those OK as they are?

From a quick look at that spec:

Starting with ICU 4.8, an ASCII apostrophe only starts quoted text if it immediately precedes a character that requires quoting (that is, "only where needed")

so I think they are. There's also this suggestion, though, which perhaps we might take up:

Recommendation: Use the real apostrophe (single quote) character ’ (U+2019) for human-readable text, and use the ASCII apostrophe ' (U+0027) only in program syntax, like quoting in MessageFormat.

There's also this suggestion, though, which perhaps we might take up:

Recommendation: Use the real apostrophe (single quote) character ’ (U+2019) for human-readable text, and use the ASCII apostrophe ' (U+0027) only in program syntax, like quoting in MessageFormat.

Interesting! Sure, we can do that. That’ll be really hard to remember to do in the future, though; I wonder what kind of automated check we might be able to put in, like in tools/test or something.

gnprice · 2020-08-10T23:12:14Z

flow-typed/react-intl_vx.x.x.js

@@ -158,7 +158,7 @@ declare module 'react-intl' {
      [key: string]: PrimitiveType | React.ReactElement<> | FormatXMLElementFn,
      ...,
    },
-  > extends React.Component<Props_3<V>> {
+  > extends React$Component<Props_3<V>> {


[…] I tried exhaustively translating everything from the `React.` namespace into a `React$` equivalent, but things broke in mysterious ways. Better to do it incrementally, then, and we can come back if we want more of it to work.

Hmm, dang.

A reasonable strategy. I'm curious what broke when you tried rewriting all of them.

There's a list of the ones with dollar signs at https://www.saltycrane.com/cheat-sheets/flow-type/latest/#lib/react.js-private.

Unfortunately, that list isn't just a translation of all React.Foo (from the react module) to React$Foo. There's some guesswork involved; e.g., does React.FC translate to React$StatelessFunctionalComponent, and what does React.PropsWithChildren translate to (or do we have to invent it ourselves)? 🤷

At first, I thought I could get all these translations right in one pass, but there were enough errors that I decided to start over and just do a few that didn't make things break. These seemed simple enough that I wouldn't be making some subtle mistranslation that might come back to haunt us later.

Hmm, and (at the tip, with v5 where the TS uses React_2 for some reason) I just now tried changing React_2.PureComponent to React$PureComponent. It broke at first...then I changed mixins to extends wherever React$Component or React$PureComponent was being extended, and it worked again.

Though I don't know why it was using mixins instead of extends; seems like it should be extends.

OK, just handled a few more React things.

It broke at first...then I changed mixins to extends wherever React$Component or React$PureComponent was being extended, and it worked again.

Weird. Looks like mixins here is some Flow syntax that isn't documented: facebook/flow#1383

gnprice · 2020-08-10T23:41:37Z

src/common/Input.js

          <TextInput
            style={[this.styles.input, { color: this.context.color }, style]}
-            placeholder={text}
+            placeholder={chunks[0]}


Interesting, seems like a bug if this callback ever does get passed several chunks. One which is easier to spot after this API change, so that's a point in the API change's favor!

(Perfectly right for this commit to just preserve the existing behavior, but then perhaps we can follow up to fix.)

We can't just pass the whole array here, because it wants a string.

From looking at the docs:
https://formatjs.io/docs/react-intl/components#formattedmessage

By default <FormattedMessage> will render the formatted string into a <React.Fragment>. If you need to customize rendering, you can either wrap it with another React element (recommended), specify a different tagName (e.g., 'div'), or pass a function as the child.

it looks like, first, this is not the recommended way to use this API 🙂 ; and second, what's going on when there are multiple chunks is that they are React nodes that aren't necessarily strings. This happens if you pretty explicitly make it happen with what you pass in values.

So I think this is not a live bug because we never do that. But probably this code would be cleaner if instead of using FormattedMessage to turn this sort of inside-out, we (a) put it right-side-out, with TextInput on the outside, and (b) on the inside, because what we actually want is a string, used formatMessage (or _ which wraps it) to get a plain old string for sure.

src/types.js

gnprice · 2020-08-11T00:48:28Z

src/types.js

+// We adapt `IntlMessageFormatValue` from the `react-intl` libdef,
+// which doesn't quite match the doc at
+// https://formatjs.io/docs/react-intl/api/#formatmessage.
+type PrimitiveType = string | number | boolean | null | void | Date;
+// (name is from libdef)
+// eslint-disable-next-line flowtype/type-id-match
+type FormatXMLElementFn = (parts: string[]) => string;
+type IntlMessageFormatValue = PrimitiveType | FormatXMLElementFn;


Ah, but also I think this can all be avoided by correcting the libdef like so -- this is something that TS doesn't know how to express, so flowgen wouldn't have been able to translate it:

formatMessage( descriptor: MessageDescriptor, - values?: { [key: string]: PrimitiveType | FormatXMLElementFn<string, string>, ... }, + values?: { +[key: string]: PrimitiveType | FormatXMLElementFn<string, string>, ... }, ): string;

The + makes the property (here, the "indexer property" so effectively all properties) "covariant" rather than "invariant" for the object type. I.e., it's fine to pass a values where all the values have type some subtype of the complicated type mentioned, not only where they all have that type. Flow's docs don't quite explain this in one coherent place, but these seem to be the relevant bits:
https://flow.org/en/docs/types/interfaces/#toc-interface-property-variance-read-only-and-write-only
https://flow.org/en/docs/types/utilities/#toc-readonly
https://flow.org/en/docs/lang/variance/

and on the general concept independent of Flow, this is a pretty good explanation:
https://www.stephanboyer.com/post/132/what-are-covariance-and-contravariance

The case where that covariant type would be wrong is if formatMessage were going to mutate the object, and then we were going to look back at it and look at the new values it put there. Then it'd be important that the caller have the exact same idea of what types are allowed for those values as formatMessage itself does. But it shouldn't be mutating the object, so covariance is appropriate.

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest all at once. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [3], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [4]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] except for React, which is normal, and which we'll address to some extent in the next commit. [4] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

chrisbobbe · 2020-08-11T21:11:02Z

OK, just pushed some changes, ready for another review!

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest all at once. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [3], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [4]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] except for React, which is normal, and which we'll address to some extent in the next commit. [4] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

chrisbobbe · 2020-08-12T06:34:44Z

(Just fixed a conflict.)

gnprice

I'm getting this red-screen when I try running the app in a debug build on Android:

At a quick bisect, that happens after the first commit of the series (the upgrade to v3).

Can you reproduce the issue? I'm guessing it's only on Android.

From the new polyfills.js:

/* @flow strict-local */

import '@formatjs/intl-pluralrules/polyfill';
import '@formatjs/intl-pluralrules/polyfill-locales';

import '@formatjs/intl-relativetimeformat/polyfill';
import '@formatjs/intl-relativetimeformat/polyfill-locales';

it sounds like it's happening in the first of those imported modules, and the issue is there's no such thing as Intl. I wonder if there's another step needed for that polyfill.

Probably a chat thread would be the best place to debug if you don't spot something obvious.

Otherwise just a minor code comment below.

gnprice · 2020-08-13T03:23:24Z

src/message/messageActionSheet.js

@@ -112,7 +112,7 @@ muteTopic.title = 'Mute topic';
 muteTopic.errorMessage = 'Failed to mute topic';

 const deleteTopic = async ({ auth, message, dispatch, ownEmail, _ }) => {
-  const alertTitle = _("Are you sure you want to delete the topic '{topic}'?", {
+  const alertTitle = _('Are you sure you want to delete the topic ’{topic}’?', {


Hmm! So I actually didn't think about the case when these are really quotation marks -- I was thinking of the apostrophes. This doesn't look right to me:

to delete the topic ’{topic}’

and I think the issue is that U+2019 is the right single quotation mark. There's also ‘ U+2018 LEFT SINGLE QUOTATION MARK.

Looking afresh at this particular message: it seems like the normal thing in English text that isn't code would be to use double-quotes, anyway. Could use either the plain ASCII ", or the Unicode quotation marks:
“ U+201C LEFT DOUBLE QUOTATION MARK
” U+201D RIGHT DOUBLE QUOTATION MARK
What do you think?

Sure, sounds good (the Unicode quotation marks).

gnprice · 2020-08-13T03:34:40Z

flow-typed/react-intl_vx.x.x.js

@@ -158,7 +158,7 @@ declare module 'react-intl' {
      [key: string]: PrimitiveType | React.ReactElement<> | FormatXMLElementFn,
      ...,
    },
-  > extends React.Component<Props_3<V>> {
+  > extends React$Component<Props_3<V>> {


It broke at first...then I changed mixins to extends wherever React$Component or React$PureComponent was being extended, and it worked again.

Weird. Looks like mixins here is some Flow syntax that isn't documented: facebook/flow#1383

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest all at once. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [3], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [4]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] except for React, which is normal, and which we'll address to some extent in the next commit. [4] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

chrisbobbe · 2020-08-24T23:15:29Z

OK, I've pushed some changes, including a fix for that Android error you've been seeing. Also posted a few notes on CZO here.

chrisbobbe · 2020-09-14T23:01:52Z

Marking as blocked by #3547; see discussion.

v7 is the latest, but might as well get there incrementally. The v6 release seems most excited to talk about its new use of the new Context API, which sounds like it might mean a lot of breaking changes we have to address manually (I guess I'm thinking of zulip#4222). But no; in fact, there's just one small announced breaking change that applies to us: - The `withRef` option to `connect` has been replaced with `forwardRef`. If `{forwardRef : true}` has been passed to `connect`, adding a ref to the connected wrapper component will actually return the instance of the wrapped component. So, make that change, including the implied removal of `getWrappedInstance`; that bit of UI works fine on Android and iOS after that removal. There's a FlowTyped libdef for this version, so, take that. An additional Flow error arose, falsely telling consumers of our `connect`-wrapped `MentionWarnings` component that they need to pass props like `auth`; in fact, those are not expected to be passed because they're provided by `connect`. Just as we've done at the `connect` call site, add a temporary $FlowFixMe until we can get `connect` properly type-checked. It looks like we don't want to linger on v6 for very long; v7 addresses some performance complaints that arose in v6. So, we'll move to v7 ASAP.

chrisbobbe · 2020-10-16T00:51:30Z

And #3547 has been resolved, so I'm unblocking this. 🙂

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest all at once. Well, not quite the latest -- with 5.8.1 and following, `react-intl` has `typescript` as a peer dependency, with the range "3.8 || 4". Unfortunately, this range is disjoint with another peer-dep range that caused us to add `typescript` in the first place; see 78daeb6. (See also formatjs/formatjs#2067 (comment) for a `react-intl` maintainer's flippant response to the idea of using peer dependencies correctly.) So, use 5.8.0 precisely. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [3], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [4]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] except for React, which is normal, and which we'll address to some extent in the next commit. [4] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest all at once. Well, not quite the latest -- with 5.8.1 and following, `react-intl` has `typescript` as a peer dependency, with the range "3.8 || 4". Unfortunately, this range is disjoint with another peer-dep range that caused us to add `typescript` in the first place; see 78daeb6. (See also formatjs/formatjs#2067 (comment) for a `react-intl` maintainer's flippant response to the idea of using peer dependencies correctly.) So, use 5.8.0 precisely. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [3], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [4]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] except for React, which is normal, and which we'll address to some extent in the next commit. [4] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

chrisbobbe · 2020-10-22T15:06:20Z

Bumping the priority of this—see #4274 (comment).

gnprice

Thanks! Just small comments below.

static/translations/messages_en.json

package.json

src/i18n/locale.js

package.json

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest (currently 5.8.6) all at once. We restrict the version range to that single version (instead of using a caret or a tilde), since `react-intl` doesn't invite its users to expect breaking changes to be handled well [3]. We also change our useless `typescript` dev-dependency's version to a range that's in the intersection of the range `react-intl` gives for it, and the range we found in 78daeb6. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [4], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [5]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] formatjs/formatjs#2067 (comment) [4] except for React, which is normal, and which we'll address to some extent in the next commit. [5] zulip#4222 (comment)

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

chrisbobbe · 2020-10-23T01:55:12Z

Thanks for the review! Just pushed a new revision.

We don't want to be using JavaScriptCore forever, if we can avoid it; we're tracking the switch to Hermes as zulip#4131. In fact, Hermes doesn't yet support Intl. For now, this tiny code change will allow `react-intl` to work on Android. A `react-intl` doc [1] links to instructions [2] [3] to use a variant of JavaScriptCore that supports `Intl`. So, follow those instructions. Reportedly, this change will add 6MiB per architecture. zulip#3547 was recently resolved, though, giving us some wiggle room. [1] https://formatjs.io/docs/react-intl/#react-native [2] https://github.com/react-native-community/jsc-android-buildscripts#international-variant [3] They're slightly out-of-date, currently, but it's still easy to tell what to do; see facebook/react-native@d7f5153cd#diff-b8dd15c78827c1b48f9fe21c45686142R100-R111 for the new edit to make to `android/app/build.gradle`.

In `react-intl` v3, the escape character is changed to the apostrophe (') [1]. So, don't use it in our text. [1] https://formatjs.io/docs/react-intl/upgrade-guide-3x/#escape-character-has-been-changed-to-apostrophe-

The old Context API is fundamentally broken, and much time has been spent engineering a way around it. This new version doesn't use it anymore, which is a relief! It's the last of our dependencies that did, at least in a way we had to be aware of. Now, our `intl` consumers will re-`render` when we expect them to -- and they'll no longer need the `key` hack. Nor will we need to "translate" (as we've been doing in `TranslationContextTranslator`) from the old to the new API. Following the upgrade guide [1], this commit does the following: - Stop using `addLocaleData`. It was removed, and we no longer need it. - No polyfilling. The section "Migrate to using native Intl APIs" suggests there may be something we have to do here, but it doesn't mention React Native specifically. There is a RN mention elsewhere, though, in the "Overview" doc [2]. So, follow that. In a recent commit, we made Android use the `Intl` variant of JavaScriptCore, and we trust a few rumors [3] (and the fact that it works for me) that JSC on iOS 10+ already has `Intl` support (our Deployment Target is 11.0), not having found a changelog or release announcement to confirm. - Remove `vendor/intl/intl.js`; we don't need it after the upgrade. It's not clear from fcc9466 why (or if) we really needed it before the upgrade. - Remove the section in our React architecture doc about the old Context API! - Change out the libdef. There wasn't one from FlowTyped, as usual, so, translate from the TypeScript ambient type files with Flowgen and do manual tweaks. - Revert 5b6014e because v3 of this package doesn't use `prop-types` [4]. After running `yarn`, though, we got a peer-dep warning from "react-navigation > [email protected]". So, undo the reversion by adding `prop-types` back in, at the same semver range, and run `yarn`. Other breaking changes that don't affect us are mentioned in the upgrade guide. Also, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`. [1] https://formatjs.io/docs/react-intl/upgrade-guide-3x [2] https://formatjs.io/docs/react-intl/#react-native [3] See facebook/react-native#19737 (comment) and facebook/react-native#19251 (comment), and discussion on CZO at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60.40formatjs.60.20polyfills/near/998279. [4] formatjs/formatjs#1382 (comment)

Anything at `React.` is `any` [1]. So, in a few places, use Flow's builtin React type annotations, like React$Node instead of React.Node. These are the obvious ones that we use. I tried exhaustively translating everything from the `React.` namespace into a `React$` equivalent, but things broke in mysterious ways. Better to do it incrementally, then, and we can come back if we want more of it to work. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/libdef.3A.20react-native-webview/near/896571

Following from the previous commit, these are all the ones I'd really feel confident and comfortable doing, above and beyond the ones I expect to be most visible. Plenty of `React.` occurrences remain. Let's fix these carefully if it turns out they're suppressing some really useful type checking, but I don't expect it to be really easy.

The upgrade guides for v4 [1] and v5 [2] are pretty short and simple, so, might as well jump to the latest (currently 5.8.6) all at once. We restrict the version range to that single version (instead of using a caret or a tilde), since `react-intl` doesn't invite its users to expect breaking changes to be handled well [3]. We also change our useless `typescript` dev-dependency's version to a range that's in the intersection of the range `react-intl` gives for it, and the range we found in 78daeb6. With v4 (and continuing into v5), we get a big stroke of luck for the libdef. There's a single TypeScript file that contains all the necessary types [4], so we only need to run Flowgen once, on that one file. This saves a lot of the kind of work we had to do for v3. The v4 declared breaking changes don't apply to us. One v5 breaking change applies to us: - `FormattedMessage` render prop is no longer variadic. We've only ever cared about the first argument passed to the render prop, so, grab just that one, to minimally handle the API change. In fact, it seems like a bug that we're not paying attention to any others that are passed. But it looks like no others are being passed, currently; we haven't explicitly made that happen. But we'll stop using this confusing pattern in an upcoming commit; see discussion [5]. [1] https://formatjs.io/docs/react-intl/upgrade-guide-4x [2] https://formatjs.io/docs/react-intl/upgrade-guide-5x [3] formatjs/formatjs#2067 (comment) [4] except for React, which is normal, and which we'll address to some extent in the next commit. [5] zulip#4222 (comment)

Not sure why the TypeScript file is calling it React_2 instead of React...anyway, handle these, like we did in two recent commits.

Following from discussion on GitHub [1]. We would have a bug if multiple chunks were passed to the render callback prop [2], since we're just using the first one. It looks like multiple chunks won't be passed, since we haven't explicitly made that happen. Glad to have this API misuse exposed by the API change in `react-intl` v5, which we took in a recent commit: "`FormattedMessage` render prop is no longer variadic". But we don't need to use this render prop pattern at all. From the docs [3]: """ If you need to customize rendering, you can either [...] or pass a function as the child. """ We just need plain old strings, without customization. So, just do the more straightforward thing we do in a bunch of places: use `formatMessage` as it's wrapped by `_`. Plumb through `_` using `withGetText` as recommended by the JSDoc for `GetText`: """ Alternatively, for when `context` is already in use: use `withGetText` and then say `const { _ } = this.props`. """ [1] zulip#4222 (comment) [2] The render callback prop's name is `children`. The "Function as Child Component" pattern is sometimes opposed because `children` is rarely an appropriate, expressive name; see, e.g., https://americanexpress.io/faccs-are-an-antipattern/. Here, it also doesn't seem to be well-typed in the libdef. I'm unsure if the "FaCC" pattern makes it especially hard to get well-typed, but it seems plausible. [3] zulip#4222 (comment)

gnprice · 2020-10-23T21:25:55Z

Merged! Thanks again.

…rray. There's no reason we'd want to mutate this array. Given that, and the fact that we're about to make `ServerMessage` a union of `ServerMessageOf<PmMessage>` and `ServerMessageOf<StreamMessage>`, we might as well be courteous to `migrateMessages`' callers by allowing them to pass an array of just one of those types. In other words, we'd like to allow this argument, which is an array, to be treated covariantly in the type of its elements. This is only possible when we the array is read-only. See discussion at zulip#4222 (comment), and in particular the example in "What about other types" at the bottom of this article: https://www.stephanboyer.com/post/132/what-are-covariance-and-contravariance.

…rray. There's no reason we'd want to mutate this array. Given that, and the fact that we're about to make `ServerMessage` a union of `ServerMessageOf<PmMessage>` and `ServerMessageOf<StreamMessage>`, we might as well be courteous to `migrateMessages`' callers by allowing them to pass an array of just one of those types. In other words, we might as well allow this argument, which is an array, to be treated covariantly in the type of its elements. This is only possible when we the array is read-only. See discussion at zulip#4222 (comment), and in particular the example in "What about other types" at the bottom of this article: https://www.stephanboyer.com/post/132/what-are-covariance-and-contravariance.

chrisbobbe requested a review from gnprice August 7, 2020 22:13

chrisbobbe force-pushed the pr-upgrade-react-intl branch from afc8e09 to 55d4236 Compare August 7, 2020 22:15

gnprice reviewed Aug 11, 2020

View reviewed changes

chrisbobbe force-pushed the pr-upgrade-react-intl branch from 55d4236 to e626280 Compare August 11, 2020 21:07

chrisbobbe force-pushed the pr-upgrade-react-intl branch from e626280 to ad8e432 Compare August 12, 2020 06:34

gnprice reviewed Aug 13, 2020

View reviewed changes

chrisbobbe force-pushed the pr-upgrade-react-intl branch from ad8e432 to f39d44e Compare August 24, 2020 23:10

chrisbobbe added the blocked on other work To come back to after another related PR, or some other task. label Sep 14, 2020

chrisbobbe removed the blocked on other work To come back to after another related PR, or some other task. label Oct 16, 2020

chrisbobbe force-pushed the pr-upgrade-react-intl branch from f39d44e to 1fa204c Compare October 22, 2020 01:22

chrisbobbe force-pushed the pr-upgrade-react-intl branch from 1fa204c to 0fc5b7e Compare October 22, 2020 01:31

chrisbobbe mentioned this pull request Oct 22, 2020

Decouple Navigation from Redux (Part 1) #4274

Merged

chrisbobbe added the P1 high-priority label Oct 22, 2020

gnprice reviewed Oct 22, 2020

View reviewed changes

static/translations/messages_en.json Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/i18n/locale.js Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

chrisbobbe force-pushed the pr-upgrade-react-intl branch from 0fc5b7e to ee41e4c Compare October 23, 2020 01:53

chrisbobbe added 8 commits October 23, 2020 14:23

i18n: Stop using ' character.

f8351f0

In `react-intl` v3, the escape character is changed to the apostrophe (') [1]. So, don't use it in our text. [1] https://formatjs.io/docs/react-intl/upgrade-guide-3x/#escape-character-has-been-changed-to-apostrophe-

react-intl libdef: Make some React type annotations useful.

31f2e3d

Not sure why the TypeScript file is calling it React_2 instead of React...anyway, handle these, like we did in two recent commits.

gnprice force-pushed the pr-upgrade-react-intl branch from ee41e4c to 2734811 Compare October 23, 2020 21:25

gnprice merged commit 2734811 into zulip:master Oct 23, 2020

chrisbobbe mentioned this pull request Oct 23, 2020

Decouple Navigation from Redux (Part 2) #4278

Merged

chrisbobbe deleted the pr-upgrade-react-intl branch November 5, 2021 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade `react-intl` to a version that uses the new Context API, then to the latest. #4222

Upgrade `react-intl` to a version that uses the new Context API, then to the latest. #4222

chrisbobbe commented Aug 7, 2020

gnprice left a comment

gnprice Aug 10, 2020

chrisbobbe Aug 11, 2020 •

edited

Loading

gnprice Aug 10, 2020

chrisbobbe Aug 11, 2020 •

edited

Loading

chrisbobbe Aug 11, 2020

chrisbobbe Aug 11, 2020

gnprice Aug 13, 2020

gnprice Aug 10, 2020

gnprice Aug 11, 2020

chrisbobbe commented Aug 11, 2020

chrisbobbe commented Aug 12, 2020

gnprice left a comment

gnprice Aug 13, 2020

chrisbobbe Aug 24, 2020 •

edited

Loading

gnprice Aug 13, 2020

chrisbobbe commented Aug 24, 2020

chrisbobbe commented Sep 14, 2020

chrisbobbe commented Oct 16, 2020

chrisbobbe commented Oct 22, 2020

gnprice left a comment

chrisbobbe commented Oct 23, 2020

gnprice commented Oct 23, 2020

Upgrade react-intl to a version that uses the new Context API, then to the latest. #4222

Upgrade react-intl to a version that uses the new Context API, then to the latest. #4222

Conversation

chrisbobbe commented Aug 7, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 11, 2020

chrisbobbe commented Aug 12, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Aug 24, 2020

chrisbobbe commented Sep 14, 2020

chrisbobbe commented Oct 16, 2020

chrisbobbe commented Oct 22, 2020

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 23, 2020

gnprice commented Oct 23, 2020

Upgrade `react-intl` to a version that uses the new Context API, then to the latest. #4222

Upgrade `react-intl` to a version that uses the new Context API, then to the latest. #4222

chrisbobbe Aug 11, 2020 •

edited

Loading

chrisbobbe Aug 11, 2020 •

edited

Loading

chrisbobbe Aug 24, 2020 •

edited

Loading