[types] Bump minimum required TypeScript version to 3.5 by petyosi · Pull Request #24795 · mui/material-ui

petyosi · 2021-02-06T08:56:30Z

Breaking changes

[core] Increase the minimum version of TypeScript supported from v3.2 to v3.5.
[@material-ui/types] Rename the exported Omit type in @material-ui/types. The module is now called DistributiveOmit. The change removes the confusion with the built-in Omit helper introduced in TypeScript v3.5. The built-in Omit, while similar, is non-distributive. This leads to differences when applied to union types. See this StackOverflow answer for further details.

-import { Omit } from '@material-ui/types';
+import { DistributiveOmit } from '@material-ui/types';

As per #24752 (comment),
built-in Omit usage should be considered a bug. There's one exception to
that in packages/material-ui-lab/src/DayPicker/PickersSlideTransition.tsx.

This change also removes the Omit export from the package. The documentation examples resort to the built-in Omit. Let me know if this makes sense.

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2021-02-06T08:58:33Z

Details of bundle changes

Generated by 🚫 dangerJS against 7228d94

oliviertassinari

Clearing the confusion between the two helpers is great. How about we update the documentation of https://next.material-ui.com/getting-started/supported-platforms/#typescript as we rely on TypeScript 3.5 now for the built-in omit?

I think that such breaking change should also have its entry in the migration guide. Alternatively, we could do it in two different PRs (1. Bump TypeScript, 2. Omit confusion). I have no specific preferences.

oliviertassinari

For the second migration notice, I think that we could have it close to this section: https://next.material-ui.com/guides/migration-v4/#supported-browsers-and-node-versions.

docs/src/pages/getting-started/supported-platforms/supported-platforms-es.md

docs/src/pages/guides/migration-v4/migration-v4.md

oliviertassinari · 2021-02-07T18:36:36Z

packages/material-ui-lab/src/ClockPicker/ClockNumbers.tsx

  isDisabled,
  getClockNumberText,
-}: Omit<GetHourNumbersOptions, 'ampm' | 'date'> & { value: number }) => {
+}: DistributiveOmit<GetHourNumbersOptions, 'ampm' | 'date'> & { value: number }) => {


We were using the built-in Omit helper previously. Why change it for the distributive one? I guess it's the same question for all the other cases. When should we use one over the other?

I went through all occurances and applied the following:

If applied to an interface, use the built-in Omit, since the distributive one has no effect;

If applied to a union type or to a generic parameter, use DistributiveOmit.

packages/material-ui/src/useAutocomplete/useAutocomplete.d.ts

packages/material-ui/src/TextareaAutosize/TextareaAutosize.d.ts

As per mui#24752 (comment), built-in Omit usage should be considered a bug. There's one exception to that in packages/material-ui-lab/src/DayPicker/PickersSlideTransition.tsx

eps1lon · 2021-02-08T15:39:36Z

Looking at it tomorrow.

oliviertassinari · 2021-02-08T17:25:31Z

I have pushed an update to mention the change of TypeScript version in the migration guide as a standalone entry.

Looking at it tomorrow.

@eps1lon Do you mean that you have approved the changes too quickly?

eps1lon · 2021-02-08T17:41:55Z

@eps1lon Do you mean that you have approved the changes too quickly?

I have not approved all changes.

If this is needed for something we can work on it in a follow-up with tests

eps1lon · 2021-02-11T11:57:58Z

Added a rationale for the bump in TS version (focusing on the link to the DT repo).
Also reverted a bunch of changes where DistributiveOmit was added. In these instance we either know that the input isn't a union or the behavior is new. If we want to change it, we should do so with a test.

eps1lon · 2021-02-11T12:00:15Z

@petyosi Thanks!

oliviertassinari · 2021-02-14T15:32:08Z

packages/material-ui-utils/src/isMuiElement.d.ts

@@ -1,4 +1,5 @@
 import * as React from 'react';
+import { DistributiveOmit } from '@material-ui/types';


@material-ui/types isn't declared as a dependency of the package. It creates this fail: mui/mui-x#1046. Solved in #24936

petyosi mentioned this pull request Feb 6, 2021

[typescript] Replace Omit type with the built-in one #24752

Closed

1 task

oliviertassinari added typescript internal Behind-the-scenes enhancement. Formerly called “core”. labels Feb 7, 2021

oliviertassinari changed the title ~~[typescript] Fix Omit / DistributiveOmit ambiguity~~ [core] Fix Omit/DistributiveOmit TypeScript ambiguity Feb 7, 2021

oliviertassinari reviewed Feb 7, 2021

View reviewed changes

oliviertassinari requested changes Feb 7, 2021

View reviewed changes

oliviertassinari requested a review from eps1lon February 7, 2021 19:57

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 7, 2021

petyosi added 2 commits February 8, 2021 10:22

[typescript] Fix Omit / DistributiveOmit ambiguity

c50e6e9

As per mui#24752 (comment), built-in Omit usage should be considered a bug. There's one exception to that in packages/material-ui-lab/src/DayPicker/PickersSlideTransition.tsx

[docs] Change the minimum TS version to 3.5

1567dba

petyosi force-pushed the distributive-omit branch from e0f0807 to 8a6f1fd Compare February 8, 2021 08:23

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 8, 2021

[docs] Document the Omit type removal + alternative

d3e2487

petyosi force-pushed the distributive-omit branch from 8a6f1fd to d3e2487 Compare February 8, 2021 08:34

petyosi requested a review from oliviertassinari February 8, 2021 08:39

eps1lon approved these changes Feb 8, 2021

View reviewed changes

eps1lon changed the title ~~[core] Fix Omit/DistributiveOmit TypeScript ambiguity~~ [types] Fix Omit/DistributiveOmit TypeScript ambiguity Feb 8, 2021

eps1lon added breaking change Introduces changes that are not backward compatible. package: core package: types Specific to types package. and removed internal Behind-the-scenes enhancement. Formerly called “core”. labels Feb 8, 2021

oliviertassinari added 2 commits February 8, 2021 16:18

yarn prettier

1d5d13e

add an entry in the migration guide

6879910

oliviertassinari approved these changes Feb 8, 2021

View reviewed changes

eps1lon self-requested a review February 11, 2021 11:14

Add rationale for change of TS version

342fe40

Revert change of existing behavior

7228d94

If this is needed for something we can work on it in a follow-up with tests

eps1lon approved these changes Feb 11, 2021

View reviewed changes

eps1lon changed the title ~~[types] Fix Omit/DistributiveOmit TypeScript ambiguity~~ [types] Bump minimum required TypeScript version to 3.5 Feb 11, 2021

eps1lon merged commit 058e409 into mui:next Feb 11, 2021

petyosi deleted the distributive-omit branch February 11, 2021 12:00

github-actions bot mentioned this pull request Feb 11, 2021

Update snapshots for pr/24795 eps1lon/mui-scripts-incubator#1842

Closed

oliviertassinari reviewed Feb 14, 2021

View reviewed changes

This was referenced Apr 24, 2021

[docs] Update minimum required TypeScript version #25930

Merged

[internal] Enable trailing comma in TypeScript files #25931

Merged

oliviertassinari added this to the v5-beta milestone Aug 17, 2021

oliviertassinari mentioned this pull request Aug 17, 2021

Deprecation warnings for v5 #22074

Closed

42 tasks

		@@ -1,4 +1,5 @@
		import * as React from 'react';
		import { DistributiveOmit } from '@material-ui/types';

Uh oh!

Conversation

petyosi commented Feb 6, 2021 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

mui-pr-bot commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviertassinari Feb 7, 2021

Choose a reason for hiding this comment

Uh oh!

petyosi Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eps1lon commented Feb 8, 2021

Uh oh!

oliviertassinari commented Feb 8, 2021

Uh oh!

eps1lon commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon commented Feb 11, 2021

Uh oh!

eps1lon commented Feb 11, 2021

Uh oh!

oliviertassinari Feb 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

petyosi commented Feb 6, 2021 •

edited by oliviertassinari

Loading

mui-pr-bot commented Feb 6, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

eps1lon commented Feb 8, 2021 •

edited

Loading