[core] fix(TagInput): restore type signature of onRemove prop

[adidahiya]

Fixes a regression introduced in #4291, released in core 3.32.0-3.32.1

I discovered this when enabling stricter compilation options. (value: string, index, number) => void is not assignable to onRemove: (value: React.ReactNode, index: number) => void, so we can't just change the type of the first param.

Longer-term, I think <TagInput> should be generically typed based on its values... or maybe just stick to string?

[blueprint-bot]

fix test expectations

Previews: documentation | landing | table

[rhysbrettbowen]

@adidahiya This just broke things for us as we render the tags as a react node. We can switch things to use the 3rd param but in future it would be better to fix the types rather than change functionality. Would also be better to have it generically typed as you said and somehow match up the actual value rather than what is rendered to be passed through.

bit of context - we have a tag renderer for a multiselect component that returns a react node to display. We inspect that node that comes back to find it's original value (we saved in an attribute on a react node) rather than the index as we may do some sorting as well.

[adidahiya]

@rhysbrettbowen sorry about the break, but I think fixing the types without changing functionality would have required a type cast:

onRemove?.((values[index] as unknown) as string, index, values[index]);

is this what you had in mind?

[rhysbrettbowen]

The main issue is (value: string, index: number) => void for TagInput#onRemove is the wrong type. It should be (value: React.ReactNode, index: number) => void. That is what TagInput has for its onChange. You provide the values to TagInput as a ReactNode, so converting it to a string for onRemove makes no sense. If it was string in the past then that was probably wrong.

Having a look back, it should have been changed in the original PR allowing react nodes: #1419. In that case I think this should really be reverted and a compile time type check failure should probably be preferred to make things correct.

[adidahiya]

Hm ok, I think you're right. The type should be ReactNode and we should just live with the "break" introduced in #4291 which actually fixed the types to reflect the real values at runtime. I will revert this commit.