[Core] Adds additional arg to Tag onRemove handler

[lucasray]

Checklist

• Enable CircleCI for your fork
• Include tests
• Update documentation

Changes proposed in this pull request:

Adds a 2nd arg to the Tag's onRemove handler. This a non-breaking change, as passing functions that accept only 1 argument will still typecheck. This is useful in order to properly handle the onRemove callback without creating a lambda or additional React component, which may have perf consequences.

[cmslewis]

Would prefer to pass just a single identifying value here, but given that there isn't a single, clearly prescribed identifier, this looks fine to me!

Requesting one small change before approving/merging.

[cmslewis]

Can we add a line to the description explaining what the props parameter is?

e.g. Receives the props for the tag that was removed.

[cmslewis]

or maybe rename to tagProps and avoid a description altogether.

[lucasray]

sure, I'll rename to tagProps, at which point I think it's fairly clear what props are being passed, but happy to also add to the description if you'd like

[cmslewis]

tagProps alone is fine to me. 👍

[llorca]

Is safeInvoke necessary here?

[lucasray]

since onRemove is effectively Function | undefined, the typechecker should guarantee this to be safe. this callback only actually occurs if isFunction(onRemove) from line 42, though that isn't expressed in the types here. what does safeInvoke provide that the typechecker cannot? happy to rewrite this whole function as:

private onRemoveClick = (e: React.MouseEvent<HTMLButtonElement>) => {
  const { onRemove } = this.props;
  safeInvoke(onRemove, e, this.props);
}

if preferred, just seems slightly more complicated for no gain.

[cmslewis]

The benefit is more for succinctness than anything. Note this whole thing becomes one line with safeInvoke:

Utils.safeInvoke(this.props.onRemove, e, this.props);

[lucasray]

@cmslewis okay, do you all prefer this style? I slightly prefer the existing (clear what is happening without prior knowledge of what safeInvoke does, very slightly more performant since it doesn't add another call), but fine with changing it

[cmslewis]

Yeah, it's BP convention at this point, but we don't need to block on it.

[lucasray]

Fixes lint

Preview: documentation
Coverage: core | datetime

[cmslewis]

Yeah, it's BP convention at this point, but we don't need to block on it.