fix: typing on IconComponent

[murrple-1]

This is a probable fix for https://github.com/vonovak/react-navigation-header-buttons/issues/240

Expand the type of IconComponent to support the types employed by react-native-vector-icons

[vonovak]

Hello, and thank you for the PR!

When you say it's a "probable" fix, how should I understand that? How did you test the change?

Thank you 👍 🙂

[murrple-1]

Strictly speaking, I didn't test it within the confines of this project exclusively - the patch was written using Github tools, not a full git clone/npm install/npm run tests-like cycle. I can do that if you require it for approval, no problem. Though I can't imagine a scenario where extending the type definition alone would break any tests, so I'm not sure if that would be useful.

However, in the project in which I encountered the issue, I was able to workaround the issue from the opposite direction - by casting to the more restrictive type:

import React from 'react';
import { ColorValue } from 'react-native';
import MaterialIcons from 'react-native-vector-icons/MaterialIcons';
import {
  HeaderButton,
  HeaderButtonProps,
} from 'react-navigation-header-buttons';

export const IconHeaderButton: React.FunctionComponent<
  HeaderButtonProps
> = props => (
  <HeaderButton
    IconComponent={
      MaterialIcons as React.ComponentType<{
        name: any;
        style?: any;
        color?: ColorValue;
        size?: number;
      }>
    }
    iconSize={23}
    {...props}
  />
);

and there were no issues encountered with using MaterialIcons like that, for what it's worth.

On the other hand, you, as the architect, may have legitimate concerns with extending the type definition, and what that means going forward for your exposed API. I have not deep-dived the project's code enough to know what those concerns may be, so this PR is a naive attempt at fixing a bug that affected "me", and hopefully others.

Hence the use of "probable" - this PR fixes a bug, but I can't know the wider implications of this fix.

[vonovak]

hi @murrple-1 and thanks for your patience :)

• the patch was written using Github tools, not a full git clone/npm install/npm run tests-like cycle

that is just fine, my question was directed at whether you made sure in your project that the change has the intended effect?

I'm happy to merge fixes, but, generally speaking, I merge fixes when there's some confirmation / indication that they work, I do not merge "probable" fixes.

Thank you and hope this makes sense. 👍

[murrple-1]

Yes, in my project, the change has the intended and expected effect.

[murrple-1]

I'll see what I can do about the lint failure...

[murrple-1]

Issue was that the typing broke @expo/vector-icons (which I'm not using) type-checking unexpectedly. New commit resolves the typing issues

[vonovak]

Thank you! :)