[Doberjohn]

Motivation

Icon's public source prop was typed any, so it accepted anything. Typing it as IconSource showed what that hid: render functions get direction and testID, which the type didn't list, and Icon passed direction: null where the types and the icons guide say 'ltr' or 'rtl'. It now passes 'ltr', which renders the same.

Related issue

None. #5194 (on 6.1.0) exports IconSource from the package root separately.

Screenshots / Videos

No UI change

Test plan

yarn typecheck, yarn lint and yarn test pass. New tests: a boolean or undefined source renders nothing and is a @ts-expect-error, and render functions get 'ltr'. The BottomNavigation custom icon tests now render icons.

[github-actions]

Found potential problems with the pull request:

• Screenshot or video evidence is missing. Make sure to include one if it affects the UI.

[alpharius-ck]

@Doberjohn Nice change. Nothing blocks the merge. These are nits:

1. source={null} still throws at runtime. typeof null === 'object', so 'direction' in source throws a TypeError. It threw before too, so it isn't a regression. But this PR is about non-icon values rendering nothing, and null is the obvious one a JS consumer would pass. Adding source !== null && to the directional check fixes it, and a third @ts-expect-error case in the "renders nothing" test would cover it.

    const directional =
      typeof source === 'object' &&
   +  source !== null &&
      'direction' in source &&
      source.direction &&
      source.source
        ? source
        : null;

2. Inconsistent guards at the call sites. DrawerCollapsedItem checks icon !== undefined, while SegmentedButtonItem checks truthiness (showIcon && icon). Both work, because Icon already rendered nothing for these values. One style would read better.

3. isImageSource type guard isn't strictly true on web. source is ImageSourcePropType also returns true for URL strings on web. The new comment explains this and it's harmless, since the narrowing only removes types in the false branch. A short note on the return type may be enough.

4. Direction logic is thinly tested. Only the render-function path is checked for 'ltr'. Nothing covers { source, direction: 'rtl' } or 'auto' reaching a render function, or the string path reaching the PaperProvider settings.icon function. The restructured directional and direction logic is the riskiest part of the diff, so a small parametrized test would be worth adding.

5. Migration note: testID is now optional. Apps that type their render-function props with a required testID: string get a new type error. The PR fixes this in its own example, but the ### Icon section of migration.md should say that testID is optional in the typed props.

[Doberjohn]

Thanks for the review, 41e24b5 addresses 1, 4 and 5:

• Nit 1: Icon now checks source !== null before the in check, and the "renders nothing" test has a null case.
• Nit 4: Two parametrized tests cover 'rtl', 'ltr' and 'auto' under LTR and RTL layouts, for a render function and for the settings.icon path with a plain name and a directional one. The types only allow a name or an image source inside a directional object, so the render function case sits under a @ts-expect-error.
• Nit 5: The ### Icon section of the migration guide now says testID is optional, with a short diff.

I left 2 and 3 as they are. For 2, both guards behave the same, since Icon already renders nothing for those values. For 3, the comment above isImageSource covers the web case, and the narrowing only removes types in the false branch, so it can't hide a real type error.