Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump typescript from 3.2.4 to 3.8.2 #19242

Merged
merged 7 commits into from
Feb 24, 2020
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 15, 2020

3.2.4 is the lowest version of typescript we support. By holding it back we made sure all types work as expected with the lowest version.

We change this in order to verify efforts made to address #19113. The build will break at first. The changes to types will be included in a separate PR to verify that the changes work for 3.2.4 as well.

The goal is to have one PR that changes (and hopefully simplifies) the types without changing the typescript version. This PR will (after the previous one is merged) only contain changes to the tests and the used typescript version.

Changing this is justifiable because of precedent: we test with the latest version of react as well. So far this worked but it's hardly compareable since typescript and react use different version schemes.

@eps1lon eps1lon added typescript dependencies Update of dependencies labels Jan 15, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 15, 2020

No bundle size changes comparing 7582461...3406717

Generated by 🚫 dangerJS against 3406717

@oliviertassinari
Copy link
Member

@eps1lon There is an interesting data point we could leverage (but to be cautious about the interpretation, does it have a signal/noise ratio high enough?). @material-ui/pickers runs with a different version of TypeScript: v3.7.2. @dmtrKovalenko What have been your TypeScript versioning strategy so far? Did you notice pros/cons?

@dmtrKovalenko
Copy link
Member

dmtrKovalenko commented Jan 15, 2020

I was trying to be consistent with material-ui but then just updated to the latest one. And did not get any problems. Actually ts is not following semver. So I think we need to just update it, cause new features are really awesome.

@eps1lon eps1lon marked this pull request as ready for review January 15, 2020 18:44
@merceyz
Copy link
Member

merceyz commented Jan 17, 2020

This would allow us to remove createStyles from all the demos

@eps1lon
Copy link
Member Author

eps1lon commented Jan 17, 2020

This would allow us to remove createStyles from all the demos

Sure but I don't think we should do it. Just like we wouldn't use any react features that aren't available in the lowest react version we support.

@oliviertassinari
Copy link
Member

Should we add a mention about the lowest version of TypeScript we support in https://material-ui.com/getting-started/supported-platforms/ (after React)?

@merceyz
Copy link
Member

merceyz commented Jan 17, 2020

Sure but I don't think we should do it. Just like we wouldn't use any react features that aren't available in the lowest react version we support.

Agreed, should probably mention somewhere that createStyles isn't needed for typescript >= 3.4
Perhaps in the JSDoc for the function itself

@eps1lon eps1lon force-pushed the test/typescript-3.8 branch 3 times, most recently from 3fd285c to 16fe089 Compare January 21, 2020 08:44
@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Jan 24, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Jan 24, 2020

Intended to be long-living. Will receive occasional merges from other PRs that change types to make sure we're not creating debt.

@eps1lon eps1lon force-pushed the test/typescript-3.8 branch 4 times, most recently from 3060e90 to 2cba1fb Compare February 1, 2020 11:04
@eps1lon eps1lon changed the title Bump typescript from 3.2.4 to 3.7.4 Bump typescript from 3.2.4 to 3.8.1 Feb 12, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Feb 12, 2020

Plan is to merge this once 3.8.2 is released.

@eps1lon eps1lon changed the title Bump typescript from 3.2.4 to 3.8.1 Bump typescript from 3.2.4 to 3.8.2 Feb 12, 2020
@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Feb 24, 2020
@eps1lon eps1lon merged commit 67befd7 into mui:master Feb 24, 2020
@eps1lon eps1lon deleted the test/typescript-3.8 branch February 24, 2020 07:57
@NMinhNguyen
Copy link
Contributor

Curious how you plan on verifying that you don't accidentally use syntax not supported by 3.2.4? Is there some tool/setting that enables this?

@eps1lon
Copy link
Member Author

eps1lon commented Feb 27, 2020

Curious how you plan on verifying that you don't accidentally use syntax not supported by 3.2.4? Is there some tool/setting that enables this?

Not yet. If we regress too often we would have to look into it. Until then typescript is treated like any other dependency. For example we're also using the latest react version and have no tooling to verify how the code behaves on older versions.

@dmtrKovalenko
Copy link
Member

BTW there is no critical breaking changes between 3.2 and 3.8 moreover if typescript will find something inconvenient it will fail to any if skipLibCheck: true.
So + to @eps1lon we should not verify users are updated to the same version as we are using.

@eps1lon
Copy link
Member Author

eps1lon commented Feb 27, 2020

there is no critical breaking changes between 3.2 and 3.8

But that is not for you to decide what's critical and what is not. If you use a particular pattern and this breaks between 3.2 and 3.3 then a red build might be critical because it prevents a deploy.

So we definitely keep supporting 3.2.4 in v4. Just like we support IE11 but don't actually run automated tests on it.

Only data point I have about usage is

Telemetry indicates 99.9% of VS Code users are 3.0 or newer

-- DefinitelyTyped/DefinitelyTyped#40050

I'd expect 3.2.4 or newer to cover at least 95%.

EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants