Icons: Extend type-checking to the icon names, and small other cleanups. #3635

gnprice · 2019-10-03T22:37:47Z

Before this change, if we write a name prop that mistypes an icon's
name, or refers to an icon that for any reason doesn't exist, Flow
won't give an error. Now it will.

Was looking at this a couple of weeks ago in reviewing #3617 and experimenting with the code. Most of these changes are a set of followups I'd drafted then, and just cleaned up now.

rk-for-zulip

I was very surprised this was still an issue! Early revisions did perform typechecking on icon names; but now it looks like Flow is treating the props parameters in the icon-component instantiations as some flavor of any. 😞

rk-for-zulip · 2019-10-03T23:49:48Z

src/common/Icons.js

- * property (which RN itself specifies to be `allowFontScaling?: ?boolean`).
+ * Upstream has a .js.flow file which is meant to describe this.  But the
+ * type written there is erroneously much narrower:
+ *  * it's missing most of the properties of `Text`;


This doesn't actually make it narrower – the upstream type isn't $Exact.

Oof, indeed -- I'd noticed that earlier, but then forgotten it.

That is of course a problem of its own 🙂 Will fix the comment to reflect that.

rk-for-zulip · 2019-10-04T00:01:22Z

src/common/Icons.js

@@ -28,46 +30,62 @@ type IconProps<Glyphs: string> = {|
  color?: Color,
 |};

+/** A variation of `IconProps` closer to what upstream's .js.flow says. */
+type IconPropsBusted<Glyphs: string> = {| ...IconProps<Glyphs>, allowFontScaling: boolean |};


Why not just import { IconProps as IconPropsBusted } from 'react-native-vector-icons'?

Or alternatively, instead import { Icon as IconBusted } from 'react-native-vector-icons', and use $Class<IconBusted<Glyphs>> (I think) in place of ComponentType<IconPropsBusted<Glyphs>> below.

Why not just import { IconProps as IconPropsBusted } from 'react-native-vector-icons'?

Hmm, indeed that seems to work! I'll do that.

I thought I'd tried something like that and it didn't, but something must have been different.

Or alternatively, instead import { Icon as IconBusted } from 'react-native-vector-icons', and use $Class<IconBusted<Glyphs>> (I think) in place of ComponentType<IconPropsBusted<Glyphs>> below.

It feels like something on these lines ought to work too, but if I try it (with Class) I get a bunch of Flow errors like this:

Cannot call makeIcon with Feather bound to iconSet because Icon [1] is incompatible with Icon [2]. src/common/Icons.js [2] 54│ iconSet: Class<IconBusted<Glyphs>>, : 90│ export const IconPlusSquare = makeIcon(Feather, 'plus-square'); node_modules/react-native-vector-icons/Feather.js.flow [1] 9│ declare export default Class<Icon<FeatherGlyphs>>;

Nominal typing? Something else? Dunno.

Possibly this is the thing I tried that I remembered as also being IconProps as IconPropsBusted.

rk-for-zulip · 2019-10-04T00:36:38Z

src/common/Icons.js

@@ -44,12 +44,12 @@ export type IconNames = FeatherGlyphs;
 export const Icon = fixIconType<IconNames>(Feather);

 /** A (type for a) component-type like `Icon` but with `name` already specified. */
-export type IconType = ComponentType<$Diff<IconProps<empty>, {| name: mixed |}>>;
+export type SpecificIconType = ComponentType<$Diff<IconProps<empty>, {| name: mixed |}>>;


I read this as a specific (icon-type), rather than a (specific icon)'s type.

I don't think it improves legibility at any use site.

I read this as a specific (icon-type), rather than a (specific icon)'s type.

You've read it correctly!

This name can probably be improved. The thing that's really wrong with the existing names is the relationship between Icon and this IconType: with those two names next to each other in the same small piece of API, it sure looks like IconType should be the type of Icon, but it's not... and in fact the difference between them runs in a completely different direction from that.

One question here is: why is "Type" in the name at all? Yes this is the name of a type... but if we had a convention that types' names said "Type", we'd have a lot more names like that. I'm taking "Type" here as parallel to the relationship in React (as described with Flow) between Component and ComponentType. One of those is a class -- so already a type -- and the other is the type of a thing like Component.

So IconType would be a natural name for the type of Icon and things like it... and this isn't that, but instead is a certain variation on that. "Specific" is one attempt to gesture at that variation.

I'm taking "Type" here as parallel to the relationship in React (as described with Flow) between Component and ComponentType.

Yes – that's the relationship it describes. It's not the type of src/common/Icons.js:Icon, but it is the type of the various React properties we have that are named Icon. (Which are thus named because they, like the Icon here, are used as React elements.)

The fact that IconType is not also the type of this Icon is infelicitous, potentially surprising, and certainly worthy of at least an explanatory comment – but it's not, I think, worth obscuring the more-widely-relevant relationship to prevent.

If we're going to rename one of them, I'd suggest instead renaming Icon to FeatherIcon. (And similarly renaming IconNames to – well, probably to FeatherGlyphs.) But that seems to address a different issue altogether, and this one only as a side-effect.

Yeah, renaming to FeatherIcon might be a good further cleanup. That would also help with the awkward mixture we have of places where Icon means

sometimes a value like IconDone or IconTerminal or IconPrivateChat, of type SpecificIconType;

other times the Icon export of this module, which has a different type -- it requires a name prop.

Precisely because it's an awkward mixture it's a little annoying to go in and fix, though. So I'm going to go ahead and merge this without blocking on that additional fixup.

gnprice · 2019-10-04T01:32:21Z

Thanks for the review -- updated! I believe I've taken care of the first two comments.

gnprice · 2019-10-11T20:24:42Z

(ping)

rk-for-zulip · 2019-10-22T17:58:44Z

Sorry! This one fell through the cracks. Looking now.

rk-for-zulip

999999 packets transmitted, 1 received, 99.9999% packet loss, time 1000000007ms

rk-for-zulip · 2019-10-22T18:08:17Z

src/common/Icons.js

+export const IconStar = makeIcon(Feather, 'star');
+export const IconMention = makeIcon(Feather, 'at-sign');
+export const IconSearch = makeIcon(Feather, 'search');
+export const IconDone = makeIcon(Feather, 'check');


These should still be explicitly typed, regardless of what the type's name ends up being — not for the sake of additional typechecking, but so that a human reader coming in to this file with either the type name or that of a known instance can quickly leave with the other.

That'd feel pretty cluttered to me -- adding : SpecificIconType to each of these lines would feel like repetitive boilerplate.

rk-for-zulip · 2019-10-22T23:47:43Z

src/common/Icons.js

@@ -44,12 +44,12 @@ export type IconNames = FeatherGlyphs;
 export const Icon = fixIconType<IconNames>(Feather);

 /** A (type for a) component-type like `Icon` but with `name` already specified. */
-export type IconType = ComponentType<$Diff<IconProps<empty>, {| name: mixed |}>>;
+export type SpecificIconType = ComponentType<$Diff<IconProps<empty>, {| name: mixed |}>>;


I'm taking "Type" here as parallel to the relationship in React (as described with Flow) between Component and ComponentType.

Yes – that's the relationship it describes. It's not the type of src/common/Icons.js:Icon, but it is the type of the various React properties we have that are named Icon. (Which are thus named because they, like the Icon here, are used as React elements.)

The fact that IconType is not also the type of this Icon is infelicitous, potentially surprising, and certainly worthy of at least an explanatory comment – but it's not, I think, worth obscuring the more-widely-relevant relationship to prevent.

If we're going to rename one of them, I'd suggest instead renaming Icon to FeatherIcon. (And similarly renaming IconNames to – well, probably to FeatherGlyphs.) But that seems to address a different issue altogether, and this one only as a side-effect.

chrisbobbe · 2020-06-09T23:36:25Z

Small bump to @gnprice on this one, as I'm getting fresh errors from Flow 0.105.0, which will be part of the RN v0.61 upgrade. In particular, the inconsistency between react-native-vector-icons and react-native's Text component's props, in particular the allowFontScaling prop. Lots of these:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/common/Icons.js:61:46

Cannot create Feather element because null or undefined [1] is incompatible with boolean [2] in property
allowFontScaling.

     src/common/Icons.js
     58│ export const IconLanguage: IconType = props => <Feather name="globe" {...props} />;
     59│ export const IconNight: IconType = props => <Feather name="moon" {...props} />;
     60│ export const IconSettings: IconType = props => <Feather name="settings" {...props} />;
     61│ export const IconRight: IconType = props => <Feather name="chevron-right" {...props} />;
     62│ export const IconPlusCircle: IconType = props => <Feather name="plus-circle" {...props} />;
     63│ export const IconLeft: IconType = props => <Feather name="chevron-left" {...props} />;
     64│ export const IconPeople: IconType = props => <Feather name="users" {...props} />;

     node_modules/react-native-vector-icons/index.js.flow
 [2] 57│   allowFontScaling?: boolean,

     node_modules/react-native/Libraries/Text/TextProps.js
 [1] 59│   allowFontScaling?: ?boolean,

And it seems like this PR is intended to address that, and some further inconsistencies.

Before this change, if we write a `name` prop that mistypes an icon's name, or refers to an icon that for any reason doesn't exist, Flow won't give an error. Since cd899bb we've had checking of this at call sites of `NavButton`, which explicitly takes a prop of type `IconNames`. This change adds such type-checking for call sites of `Icon` itself, and at the many references to specific icons in this file.

This name is more... specific about what this particular type means.

Ran across one of these which was briefly confusing, and figured it'd be good to take a minute to clean them out. Done with the following command: $ git grep -Pho 'Icon\w+' \ | sort | uniq -u \ | grep -vFf - src/common/Icons.js \ | sponge src/common/Icons.js

gnprice · 2020-06-10T00:18:20Z

Thanks for the ping! Merged; just had to resolve some conflicts on rebase.

Hopefully that takes care of the issue you're seeing with the new Flow, too; if not we can debug in chat.

chrisbobbe · 2020-06-10T00:19:10Z

Hopefully that takes care of the issue you're seeing with the new Flow, too; if not we can debug in chat.

Yep, it absolutely did! Thank you!

gnprice requested a review from rk-for-zulip October 3, 2019 22:37

rk-for-zulip reviewed Oct 4, 2019

View reviewed changes

gnprice force-pushed the icons branch from 6d65335 to 0d3447a Compare October 4, 2019 01:31

gnprice force-pushed the icons branch from 0d3447a to 94deec5 Compare October 11, 2019 20:22

rk-for-zulip reviewed Oct 23, 2019

View reviewed changes

chrisbobbe mentioned this pull request Jun 9, 2020

Upgrade to RN v0.61 #3781

Closed

gnprice added 4 commits June 9, 2020 16:54

Icons [nfc]: Clarify the comment on IconProps; make jsdoc.

fe0688f

Icons [nfc]: Rename IconType -> SpecificIconType.

bfb0c73

This name is more... specific about what this particular type means.

Icons: Cut unused icons.

636c57c

Ran across one of these which was briefly confusing, and figured it'd be good to take a minute to clean them out. Done with the following command: $ git grep -Pho 'Icon\w+' \ | sort | uniq -u \ | grep -vFf - src/common/Icons.js \ | sponge src/common/Icons.js

gnprice force-pushed the icons branch from 94deec5 to 636c57c Compare June 10, 2020 00:02

gnprice merged commit 636c57c into zulip:master Jun 10, 2020

gnprice deleted the icons branch June 10, 2020 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

gnprice commented Oct 3, 2019

rk-for-zulip left a comment

rk-for-zulip Oct 3, 2019

gnprice Oct 4, 2019

rk-for-zulip Oct 4, 2019

gnprice Oct 4, 2019

rk-for-zulip Oct 4, 2019

gnprice Oct 4, 2019

rk-for-zulip Oct 22, 2019

gnprice Jun 10, 2020

gnprice commented Oct 4, 2019

gnprice commented Oct 11, 2019

rk-for-zulip commented Oct 22, 2019

rk-for-zulip left a comment

rk-for-zulip Oct 22, 2019

gnprice Jun 10, 2020

rk-for-zulip Oct 22, 2019

chrisbobbe commented Jun 9, 2020 •

edited

Loading

gnprice commented Jun 10, 2020

chrisbobbe commented Jun 10, 2020

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

Icons: Extend type-checking to the icon names, and small other cleanups. #3635

Conversation

gnprice commented Oct 3, 2019

rk-for-zulip left a 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

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

gnprice commented Oct 4, 2019

gnprice commented Oct 11, 2019

rk-for-zulip commented Oct 22, 2019

rk-for-zulip left a 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 Jun 9, 2020 • edited Loading

gnprice commented Jun 10, 2020

chrisbobbe commented Jun 10, 2020

chrisbobbe commented Jun 9, 2020 •

edited

Loading