Tooltip title type change in 4.9.10 breaks applications with react-i18next

[kumarunster]

In reference to #20537, the issue is already closed and comments are not possible anymore. I hope we can clarify here some questions.

This change 50c0bbc#diff-e85d391d0f501804b70512f4083dd2ecR96-R99 on the line
https://github.com/mui-org/material-ui/blob/99ca172ffd3c775240e710f647bb7880c328f7c7/packages/material-ui/src/Tooltip/Tooltip.d.ts#L99

breaks applications which uses react-i18next, because now title property excludes undefined as possible value.

Could someone explain, why it is important, where all other html wrapper types in material ui still allow React.Node as type? What was the driver for that change? Unfortunately I was not able to find anything related in corresponding issue.

Currently fix in downstream applications means ugly workarounds and search replacements in entire code base, only to fix something that was working fine all other previous versions.

Maybe you can rethink that change and revert back that line? What should happen, if the value would be undefined? What is the impact, it seems for me just an empty Tooltip.

@eps1lon, @oliviertassinari, @bartlomiejzuber

Thanks in advance.

[eps1lon]

Could someone explain, why it is important, where all other html wrapper types in material ui still allow React.Node as type?

It synced the typescript types with the prop-types. Tooltip without a title always issued a runtime dev-only warning. The requirement was documented prior to that change.

The reason is that a Tooltip without a title doesn't do anything. Not rendering a Tooltip would make more sense in these situations.

[oliviertassinari]

#20342 (comment) I propose "won't fix".

[bartlomiejzuber]

@kumarunster As a workaround you can override the i18next type of TFunction. Although I didn't had time to dig deeper into i18next to ensure that undefined can be excluded from their types hence lack of any PR made into their repository updating those.

Here's workaround I've used:

// globals.d.ts
import { TFunctionResult, TFunctionKeys, StringMap, TOptions } from "i18next";

// overridden
export type CustomTFunctionResult = Exclude<
  TFunctionResult,
  undefined | null | object
>;

declare module "i18next" {
  export interface TFunction {
    // basic usage
    <
      TResult extends CustomTFunctionResult = string,
      TKeys extends TFunctionKeys = string,
      TInterpolationMap extends object = StringMap
    >(
      key: TKeys | TKeys[],
      options?: TOptions<TInterpolationMap> | string
    ): TResult;
    // overloaded usage
    <
      TResult extends CustomTFunctionResult = string,
      TKeys extends TFunctionKeys = string,
      TInterpolationMap extends object = StringMap
    >(
      key: TKeys | TKeys[],
      defaultValue?: string,
      options?: TOptions<TInterpolationMap> | string
    ): TResult;
  }
}

[oliviertassinari]

@bartlomiejzuber Thanks for sharing.

[kumarunster]

thanks all for the answers, anyway it is a breaking change in a BUGFIX version. could you please revert, it does not make any sense to have this restriction. In my opinion such changes should be propagated in major versions. Ofcourse, it is your product, but such things are producing LOT of WTF, because nobody expecting changing types in a relatively mature product, just by bumping a third number after the dot.

[eps1lon]

I understand the frustration. It's unfortunate that this causes so many issues for you.

It was a bug since we document that it doesn't accept undefined and issued runtime dev-only warnings if the title was undefined.

It's unreasonable for types to bump major version every time we refine or fix a type. We're following @types/ versioning strategy in this regard.

[djensen47]

It's unreasonable for types to bump major version every time we refine or fix a type. We're following @types/ versioning strategy in this regard.

I know open source is hard. It gets harder when your library is popular and you have to make unpopular decisions. You also have to listen to users complain why something isn't the right way. Sorry, I try to be a productive member of open source but every so often, some things are just wrong, IMHO.

Just because @types does/did something a certain way doesn't mean it should be perpetuated. A breaking change is a breaking change. You're basically asking all of your TS users to freeze their versions at a specific patch release because your policy to allow changes to types at any release. Please correct me if I'm wrong here. Thanks. And thanks for the great library (even if I disagree with this one thing).

[oliviertassinari]

@djensen47 Call it a bug fix :)

[djensen47]

@djensen47 Call it a bug fix :)

I think there's a joke in there but somehow I missed it. 🤷‍♂️ Well, at least I can laugh at myself for not getting the joke. 🤣 🤣

[eps1lon]

You're basically asking all of your TS users to freeze their versions at a specific patch release because your policy to allow changes to types at any release.

We only ask the ones that relied on that bug. That's the great thing about versioned packages. If you're interested in a behavior that the authors consider a bug you can always keep using the older version or start forking.

So far we've had 3-4 persons come forward and all of these use cases are invalid in our opinion (an empty tooltip does not make sense). If this breaks a valid, common pattern we definitely need to rethink. But we have to target a larger population not every single use case. That's not a hard problem but a humanly impossible problem

Just because @types does/did something a certain way doesn't mean it should be perpetuated.

Definitely. But I have only a single reference point how to do things and that approach is used by every TypeScript user. If you have a better approach then I'd suggest you implement it and try it out. If the current approach is so destructive as you make it out to be then your approach should be adopted pretty fast.

[kumarunster]

@djensen47 this is just another example of JS dev world. tbh I never experienced such things in Java/Python environments, or at least I can't remember much. But here it is more "OK" to break things and semver has no meaning. remember disaster with core-js, left-pad, is-promise, react router etc.

for this case, @eps1lon already provided a hint how to shut up the typescript compiler: see here: i18next/i18next#1435 (comment)

[eps1lon]

But here it is more "OK" to break things and semver has no meaning. remember disaster with core-js, left-pad, is-promise, react router etc.

I don't appreciate these statements.

We do follow SemVer. This was a bug because it didn't work like we documented. It was only breaking if you used it in an undocumented way. SemVer clearly states that this is a SemVer patch release not a SemVer major release.

Additionally these incidents have nothing to do with SemVer. left-pad is no longer possible after npm added additional measures. The is-promise author wrote a post-mortem explaining why they made the mistake and how they'll avoid it in the future. Not sure what you mean by react-router.

Please don't handwave issues with other libraries as "breaking things is acceptable". If you have a problem with those libraries you're free to open issues on their repositories. But our issue tracker is not a platform to gossip about other free, open source libraries.

[kumarunster]

when you'll start to revert this "bugfix", how many issues needs to be opened for same?

Instead to fix documentation you decided to break type definition with bugfix version. Did javascript code changed on tooltip? Would it work differently, if plain javascript would be used? both no.

It was just type change, which caused a break in build systems, caused people to search for the issue reason, caused lot of frustration, time spent on github writing and reading issues/answers, going over project depenendecies and analyzing whats really changed. You say it is only few of us and other libraries are "wrong". But at the end it was no bug in the underlying javascript code, nor really a need from user perspective to introduce that change in the type definition.