Material-ui: Can the typings be simplified to improve performance?

[amcasey]

As suggested by @eps1lon in #18128, I'm creating this issue as a place to discuss the Material-UI typings and whether they can be simplified to reduce the amount of time spent checking them, especially during editing.

There's always a tension between having the most exact types (which provide the best errors and editor completions) and having the fast type checking (the far end of the spectrum being any).
Issues like https://github.com/microsoft/TypeScript/issues/34801 suggest that Material-UI might benefit from relaxing the exactness in order to gain back some perf.

From the repros I've investigated so far, a lot of the slowness seems to come from the large number of CSS property names (see https://github.com/mui-org/material-ui/blob/master/packages/material-ui-styles/src/withStyles/withStyles.d.ts). Not being an active CSS user myself, I have some naive questions:

1) Am I correct in assuming that having a name and type for each well-known CSS property is incredibly valuable and isn't something we could give up?
2) The CSSProperties type appears to exist to support "pseudo selectors and media queries", which - according to my limited reading - seem to be named bags of additional CSS properties.
a) Are these bags themselves recursive or is there only a single additional layer? That is, do you go from width to foo.width or to foo.bar.width, etc? If it's just one level, simplifying the types cuts my local repro from 4.6 seconds down to 3.6 seconds (i.e. big win).
b) I played around with the types myself and couldn't come up with anything better than BaseCSSProperties[keyof BaseCSSProperties], but - as I'm guessing you're aware - that's not a very useful type. It basically says that any CSS property can have the type of any (other) CSS property - that's only slightly better than any.
3) In StyleRules, if there are no properties, you get either CSSProperties or () => CSSProperties (which I will sloppily call "thunked CSSProperties"), which makes sense - the CSSProperties might be lazy. If there are properties, you get either CreateCSSProperties<Props>, which makes sense - the Props might be required to compute the CSSProperties - or (props: Props) => CreateCSSProperties<Props>, which I didn't understand because it's effectively double-lazy - you have pass in the Props once to get the CreateCSSProperties and then again to get individual properties. Why is it "double thunked"?

Separately, I suspect, but have yet to demonstrate that IsEmptyInterface is too expensive for the benefit it provides. However, it's quite possible that I don't fully understand the benefits, so it would helpful to hear more.

Can we work together to find the right balance between accuracy and perf? (Note: "just make the compiler faster" is obviously a viable strategy, but I'd like to get the typings to a good place before we optimize for them.) Thanks!

[beholderrk]

I'm not familiar with material-ui types but try to answer these questions.

1. Yes, full support for all css properties declared in actual web standards is useful.
2. a) In our case we never use a depth more than 2, but cases like this one are quite possible
   ```typescript
   const styles = (theme: Theme) =>
   createStyles({
   somediv: {
   '&:hover button': {
   visibility: 'visible',
   opacity: 1,

                ':after': {
                    content: 'x',

                    [theme.breakpoints.up('lg')]: {
                        content: 'close',
                    },
                }
            },
        }
    });
```
b) I do not understand why `BaseCSSProperties[keyof BaseCSSProperties]` is needed there

1. I think (props: Props) => CreateCSSProperties<Props> is not needed, we excluded this type in our version of material-ui types, and nothing bad happened.

It might be worth looking at the implementation of types in version 3.9.3, because in this version, type checking was fast enough and typing was good.

[eps1lon]

First of I'd like to thank you for reaching out and looking into this. It's incredibly helpful to have someone benchmark which parts of the types are slow or not.

1. Am I correct in assuming that having a name and type for each well-known CSS property is incredibly valuable and isn't something we could give up?

It's impossible to be unbiased here. I don't think we can give up on it though looking at the wider ecosystem. Chrome devtools have this feature, react itself types the style prop with CSSProperties etc. I can't see myself switching from IDE to browser and check if it was text-decoration or font-decoration or text-transform.

1. [...]
   Are these bags themselves recursive or is there only a single additional layer?

I would need to check the CSS-in-JS solution we are using. Technically media queries in CSS can be recursive. I'd be willing to cut this recursiveness and see if we get reports. Technically nested media queries can be flattened with the and operator. We should limit it to two levels though: One for media queries and one for pseudo selectors. This should still be type checked IMO:

const styles = {
  root: {
    '@media (max-width: 12cm)': {
      ':hover': {}
    }    
  }
}

Is this something you see yourself writing @oliviertassinari?

1. [...]
   CSSProperties - or (props: Props) => CreateCSSProperties, which I didn't understand because it's effectively double-lazy - you have pass in the Props once to get the CreateCSSProperties and then again to get individual properties. Why is it "double thunked"?

If the argument itself is not a property bag but a function then it requires a theme. The styles can depend on two different kind of property bags: The theme (available via React's context API) or the props (directly passed to the component):

makeStyles({ root: { color: 'blue' }}); // A
makeStyles(theme => ({ root: { color: theme.color } })); // B
makeStyles({ root: props => ({ color: props.color})}); // C
makeStyles({ root: { color: props => props.color } }); // D: same as C, only exists for dev ergonomics
makeStyles(theme => ({ root: props => ({ color: props.color || theme.color }) })); // E: what you called "double-lazy"

It's less about lazy evaluation to improve perf but more about having to wait for context and props being available.

Separately, I suspect, but have yet to demonstrate that IsEmptyInterface is too expensive for the benefit it provides. However, it's quite possible that I don't fully understand the benefits, so it would helpful to hear more.

There is one case where this is used:

Consider

const useStaticStyles = makeStyles({ root: { color: 'blue' } });
const useDynamicStyles= makeStyles({ root: { color: props =>  props.color } })
function Component() {
  const staticClasses = useStaticStyles(); // No error
  const throwingClasses = useDynamicStyles(); // $ExpectError
  const dynamicClasses = useDynamicStyles({ color: 'blue' });
}

To infer the call signature of the function returned from makeStyles (i.e. the hook named something like useSomeStyles). We need to check what kind of style bag is passed to makeStyles. We already have a helper to infer the types of props used in the style bag. If the style bag is static i.e. TS infers the {} type. Then we check the inferred type of the Props with IsEmptyInterface and for one branch we use a call signature with 0 parameters and for the other branch we use a call signature with 1 parameter that is equal to the inferred props type (see StylesRequireProps and StylesHook.

In short: We avoid having to write useStaticStyles({}) or useStaticStyles(null as any). It was introduced in https://github.com/mui-org/material-ui/pull/14019 to close https://github.com/mui-org/material-ui/issues/14018. I guess we can short circuit determining the call signature. Maybe overload the call signature instead of using conditional types?

The problem with this "feature" is that advanced users don't have a problem using null as any if they understand why. Maybe even passing an empty object is ok even though not needed. However, it's very confusing/frustrating when not used to Material-UI or TypeScript. Especially since most of the styling is not based on props anyway.

It seems like most of the time the type checker is occupied with the styling solution? I would've expected the call signature for our components i.e. what props are possible to take most of the time.

I can crawl more repositories to look specifically at usage of withStyles or makeStyles. So far I've only looked at props usage.

[oliviertassinari]

Is this something you see yourself writing @oliviertassinari?

@eps1lon We have a couple of occurrences of this nesting in the codebase (e.g. with @media (hover: none)). But it's not very frequent for the core components. I imagine it's the same userland. It could definitely be part of the tradeoff.

[amcasey]

@beholderrk

1. I figured as much, but thought I might as well ask.
2. a) Depth two should be expressible - there'll just be a bit more duplication than depth one.
   b) It took me quite a while to get my head around this and I wish I could explain it in person, instead of in text. Basically, the index signature is saying that all properties have the same type. That can only work if it specifies a type that works for all of them. One way to do that is to construct the union of all of the property types, BaseCSSProperties[keyof BaseCSSProperties] - then it will be true that every property has a compatible type. For example, suppose the only properties you could have in CSS were name: string and width: number. One way to specify an index signature that works with both properties would be to say that every property is a string | number. It's not great - name will never be a number and width will never be a string - but it works. The real problem is that the thing you want can't actually be expressed (at least, as far as we've been able to determine - there might be a "clever" hack that does it). You actually want to say that your type contains name: string, width: number or x: CSSProperties, where x is anything but name or width - it's the "anything but" that's missing. I hope that's a little clearer.
3. It sounded like @eps1lon had something to say about this, but I'm still parsing his response.

A known-good baseline would be very helpful. Do you happen to have a link?
Edit: found it.

[amcasey]

@eps1lon Happy to be part of the conversation. Fast, correct types are good for everyone. 😄

1. No argument here - I just figured I'd ask early because it would have short-circuited the whole conversation.
2. Two layers seems doable (with the notable caveat that the types will still be largely useless). I'll see if I can draft something.
3. With my lack of context, it wasn't clear that themes and props were different. If they are, then the structure makes sense. Thanks for clarifying.

Regarding isEmptyInterface, could you just make the properties parameter to (e.g.) useStaticStyles optional? It would be less correct, in the sense that callers could omit them when they're expected, but it would be a lot cheaper to type check. As I said, I don't have numbers or anything for you - it's just speculation on my part.

What are your thoughts on 2(b)? It seems like that type isn't providing much value since it basically says that any property name will be accepted and the return type can be one of a large number of things.

[eps1lon]

Regarding isEmptyInterface, could you just make the properties parameter to (e.g.) useStaticStyles optional?

I'd like to experiment with overloading the call signature first and see if this has any impact. Then we'll try making it less sound by making it always optional. It seems safer than usual to make it less sound since almost all use cases only call it once so you'll likely only make the mistake once and it surfaces pretty quickly. It's going to be tough to sell it though if it does not gain us much performance. I'll use the repository created for the original issue (https://github.com/microsoft/TypeScript/issues/34801#issue-514055289).

What are your thoughts on 2(b)? It seems like that type isn't providing much value since it basically says that any property name will be accepted and the return type can be one of a large number of things.

Skipped that accidentally. I'll look at it after the IsEmptyInterface experiment.

[amcasey]

@eps1lon Totally agree - keep isEmptyInterface if eliminating it isn't a substantial perf win.

[eps1lon]

With #19320 we got rid of some complex conditional types where function overload did achieve the same thing (removing IsEmptyInterface and any boolean logic type). Though it seems that didn't gain us much besides having less code.

I want to add that I'm currently switching back and forth between TS 3.2.4 and 3.7.4. Our type test suite runs 50% slower in 3.7.4 compared to 3.2.4 (~90s vs 50s).

I'll continue investigating if we can limit the depth of CSSProperties and removing support for media queries and pseudo selectors entirely. Having those not typed is not really an option though. The type checker should be able to check those in a reasonable time.

It might be that the type checker is the actual bottleneck. Maybe we can investigate in which version perf got hit.

[amcasey]

If you send me the commands you're running in 3.2.4 and 3.7.4, I can profile locally. However, experience suggests that the cause will probably turn out to be additional, desirable checking added since 3.2.4. (And I assume "0s" is a typo - probably "40s" or "50s"?)

Regarding CSSProperties, I agree that keeping the property names and types is extremely valuable. However, I think there's more to it than checking them in a reasonable amount of time - the first problem is that the type system can't actually express the shape you want. I think we're probably going to be spending some time working on a general solution to the problem - can I assume you'd be interested in participating in that discussion?

We have scripts for comparing multiple versions of the compiler so, if you can identify a fairly stable test you'd like us to run, we can figure chart the slowdown over time.

[eps1lon]

(And I assume "0s" is a typo - probably "40s" or "50s"?)

Sorry, it is 50s.

If you send me the commands you're running in 3.2.4 and 3.7.4, I can profile locally.

It's just yarn typescript in the root which runs the same command in every workspace that implements it. For example yarn workspace @material-ui/styles run typescript tests our types with tslint and dtslint's $ExpectError. In 3.7.4 we encountered some failures and had to adjust our tests (see #19242)

the first problem is that the type system can't actually express the shape you want.

I suspected as much. It looks like the way we mix a "concrete" shape with an object with an index signature is merely a workaround.

can I assume you'd be interested in participating in that discussion?

Definitely. I'll spend a little bit more time with non-recursive CSSProperties and then write a bit more tests to illustrate what we're looking for in those types. I would suspect that other css-in-js styling solutions hit similar performance bottlenecks.

[amcasey]

I'll try out those commands tomorrow (I suppose I should mention I'm on pacific time and observe US holidays).

It looks like the way we mix a "concrete" shape with an object with an index signature is merely a workaround.

Thanks, I've been struggling with how best to express that. Yes, you're correct that the index signature is not doing what you want. We have some thoughts on a variant that might, but we need to explore the performance implications.

I would suspect that other css-in-js styling solutions hit similar performance bottlenecks.

Very much so. We're hoping that anything we do for you can be generalized to improve the whole ecosystem.

[amcasey]

I feel like I'm missing something obvious, but I'm currently stuck. First, I gave up on Windows - things seem to work better on Linux. Let me know if you'd like to dig into that. Second, I can get yarn typescript to run - cleanly, as far as I can tell - but it appears to be running tslint, rather than pure tsc. When I run tsc on the same tsconfig.json (I'm specifically testing with styles), I get ~40 errors. What am I doing wrong? For profiling purposes, getting the repro down to a single tsc invocation would be very helpful.

[eps1lon]

@amcasey yarn typescript isn't about compilation but testing our types. We're using a similar setup to the one used in the DefinitelyTyped repo. TypeScript files in packages/* are almost always just a bunch of statements that should either pass or fail which we catch with $ExpectError.

I think the best "real-world" test case is using tsc on our docs via yarn workspace docs run tsc -p tsconfig.json after you added skipLibCheck: true and noEmit: true to ./docs/tsconfig.json:

--- a/docs/tsconfig.json
+++ b/docs/tsconfig.json
@@ -3,6 +3,8 @@
   "include": ["types", "src/pages/**/*"],
   "compilerOptions": {
     "allowJs": false,
-    "noUnusedLocals": true
+    "noUnusedLocals": true,
+    "noEmit": true,
+    "skipLibCheck": true
   }
 }

[amcasey]

@eps1lon Thanks for clarifying. I'm not thrilled that tslint got slower, but I'd like to focus on one variable at a time. I'll run the docs build you suggested with various versions of typescript and see if anything jumps out. Thanks!

[amcasey]

This setup is perfect. I'm seeing the check time double between 3.3 and 3.4.

| Version | Check Time |
|-|-|
| 3.2 | 16.71s |
| 3.3 | 16.79s |
| 3.4 | 35.25s |
| 3.5 | 21.40s |
| 3.6 | 23.10s |
| 3.7 | 27.39s |

[amcasey]

I'll dig a bit more, but I'm told that the 3.3 implementation of conditional types was incomplete, the 3.4 implementation was slow, and the 3.5 implementation is good. So, unfortunately, this is probably expected.

Specifically, I suspect this change introduced the slowdown as described in this bug.

[embeddedt]

I find it concerning that between 3.5 and 3.7 there was a 6-second increase in the time it takes to run the check. That looks pretty substantial.

[amcasey]

@embeddedt Those numbers are from single runs with each version, so there's probably quite a bit of noise. However, I'll dig in and see if I can find anything.

[amcasey]

I redid it on a Linux VM and 3.7 was consistently 20-25% slower than 3.5.

[amcasey]

This has proven to be quite difficult to bisect because consecutive runs of the same build vary by ~5% and the difference between 3.5 and 3.6 or between 3.6 and 3.7 is only ~10%.

One suspicious thing I have noticed is that styled-components provides separate .d.ts files for TS >= 3.7, so the comparison may not be apples to apples.

[amcasey]

Much to my surprise, the new styled-components types appear to be faster. Comparing apples to apples will still make the investigation easier though.

[amcasey]

I couldn't think of a clever solution, so I'm going to plot compilation time merge by merge and look for spikes. I hope to have numbers tomorrow.

[WonderPanda]

@amcasey Thanks for your efforts in looking into this! Really neat to see members of the TS team and Material UI working together. I stumbled onto this Github issue trying to figure out why our editor experience with Material UI is so slow (we're using it in two projects at work). Can definitely confirm that we're seeing a pretty significant impact in intellisense and usability inside VsCode.

At this point we'd be happy to trade a little bit of type safety in our JSS for snappy feedback for the rest of the library. Sometimes it takes 8-10 seconds of waiting before the Typescript server catches up to code changes

[amcasey]

Even with three-run averages, the data is very noisy. However, there appears to be a noticeable drop in runtime at https://github.com/microsoft/TypeScript/commit/ad322a561a301ae357da051b9221b2222c13be36 a noticeable increase (back to roughly the previous level) at https://github.com/microsoft/TypeScript/commit/480b73915fdd805952fd355e4cf3e1bc803e0878 and a general upward trend after that (though it looks too uniform to me and I suspect environmental factors) including a particular spike at https://github.com/microsoft/TypeScript/commit/c5e6d95e930048a033868d72440a9296904a33ec. I'm going to focus on the first two until I run some more tests to confirm the general upward trend (how could it get uniformly worse every commit?).

[amcasey]

The upward trend is standing up to scrutiny and I have no idea what could cause that. Maybe the discrete slowdowns are so close together that the noise makes them look like a single curve.

[amcasey]

I bumped up to 10 runs per commit and now there are four distinct regressions in the sloped area. :smile:

https://github.com/microsoft/TypeScript/commit/26caa3793e310e271ddee8adc1804486e5b0749f (~700ms)
https://github.com/microsoft/TypeScript/commit/250d5a8229e17342f36fe52545bb68140db96a2e (~500ms)
https://github.com/microsoft/TypeScript/commit/7ce793c5b8c621af5ce50af0ca3958c7bd6541bf (~1300ms)
https://github.com/microsoft/TypeScript/commit/28050d5c47c6cd7627555f12cf13b1062f80322a (~400ms)

(Total time before regressions started was ~33s.)

Just to temper expectations a bit: it's quite likely that several of these regressions will turn out to be valuable additional checks and, even if we managed to get all those checks for free, we'd still only take 20% off "much too long".

Edit: I've updated the links. Because I was going backwards through time, I got confused and flagged the merge _before_ each regression.

[amcasey]

@eps1lon Someone on this end suggested that removing @ts-ignore might help. Basically, when an error is detected, it's assumed that the user's top priority is getting good information about the error, so it's possible that a bunch of extra work will be done. If that information is subsequently dropped (because of @ts-ignore), then that time is just wasted. (Unfortunately, it's not straightforward to detect that an error does not need to be detected.) An alternative strategy is to add explicit type assertions (including to any) until the compiler stops complaining.

[eps1lon]

@eps1lon Someone on this end suggested that removing @ts-ignore might help.

We only had a single usage in docs/. I removed it anyway just in case: #19504

To be honest it sounds like @ts-ignore is an anti-pattern in that case. Not only does it not give you useful information i.e. what kind of error do we have but it also is a perf bottleneck.

As far as I can tell @ts-ignore is only useful in .js files that are being type-checked.

[amcasey]

Hmm, that almost certainly doesn't explain why the PR improving error messages regressed performance. Definitely looks like an improvement though. 😄

[amcasey]

Bugs for the changes regressing performance:
https://github.com/microsoft/TypeScript/issues/36562
https://github.com/microsoft/TypeScript/issues/36564
https://github.com/microsoft/TypeScript/issues/36565
https://github.com/microsoft/TypeScript/issues/36566
https://github.com/microsoft/TypeScript/issues/36567

Fair warning: some of these are likely to be determined to be worthwhile.

[embeddedt]

I'm quite new to the world of TypeScript, particularly when it comes to styling-in-JS solutions, but I've been quietly following this thread over the last month and I was curious to hear people's thoughts on a potential workaround, at least in the short-term.

I assume that some users (myself being one) don't make heavy use of the built-in styling system, or avoid it entirely. I do that mainly because I'm far more familiar with plain CSS or Sass and I haven't really gotten time to dig into how to effectively use the JS styling system. I prefer to just make up my own class names, write a separate stylesheet file, use the class names inside my React components and carry on. Essentially, I style as though I was writing plain HTML.

That being said, is it possible/practical to add a flag that turns off type checking for the expensive parts of styling system (possibly by conditionally doing things like type CSSProperties = any)? I don't know the statistics for the numbers of people using the style system vs. not using it, but I figure that it can't really do much harm (provided that a check was added to test the typings with that flag set), and it would be a quick way to improve performance for at least one segment of users.

Just wanted to mention the general idea; feel free to shoot it down. :slightly_smiling_face:

[amcasey]

@embeddedt Generally speaking, explicitly marking something as any is a good way to disable type checking for that symbol. Having said that, I can't recall whether you can just clobber a previous declaration or rather, as you suggest, you'd need compiler support to avoid duplicate declaration issues.

[amcasey]

New numbers (different machine, different time metric):

| Version | Total Time |
|-|-|
| 3.5.3 | 32.5s |
| 3.7.5 | 35.9s |
| master | 29.9s |

Some of the issues I posted above are still open, but we're basically back to 3.5 perf (for this benchmark). Obviously, it's still much slower than we would like, but the next batch of changes are likely to be on the material-ui side.

[eps1lon]

Tested 3.8.1 on our test suite and it seems like they're as fast as the previous ones using 3.2.4 (3.7 was significantly slower).

[amcasey]

Frankly, I can't believe how much perf we were able to claw back without giving up the new functionality. :smile: I think there might be a bit more slack (e.g. https://github.com/microsoft/TypeScript/pull/36754), but I still suspect that the most impactful change would be a simplification of the CSSProperties types. Have you had a chance to play around with those at all? It seems like at least a subset of users (e.g. @embeddedt) would be happy to give up some type checking in exchange for a perf win.

[eps1lon]

It seems like at least a subset of users (e.g. @embeddedt) would be happy to give up some type checking in exchange for a perf win

Didn't someone from the TS team just tweeted recently that for every user who wants stricter types there's one who wants looser ones? :smile:

For me it's not really about type checking (browser devtools have far better capabilities of checking your CSS). It's more about having auto-complete available.

I'll play around with different "versions" and see how they do.

[eps1lon]

@amcasey I don't think the recursive nature of CSSProperties (called CSSObject in styled-components) is the main issue. Otherwise their perf would be as bad as ours. See https://github.com/eps1lon/mui-types-perf/pull/6 and the benchmark logs.

It might be more of an issue with the amount of "overloading" we do. styled-components only allows a static object while we allow a thunked version as well as each property being a thunk. I'm not sure though.

I would love to simplify the type signature (since this is IMO also easier for devs to grasp) but I was explicitly asked to match the JSS implementation. Now that we support it, we can't easily roll back. Especially if the gain is in the 20% region. I don't think that justifies breakage.

[embeddedt]

Can the types conditionally be made less accurate (based on a user setting)? That shouldn't break any existing code (because the setting can be off by default), and would enable users like me who don't need the complex types to quickly see a performance increase.

[eps1lon]

would enable users like me who don't need the complex types to quickly see a performance increase.

It's not even confirmed that you would gain a noticeable difference. I've seen 15% perf wins but it's questionable if this is recognizeable.

The "user setting" I could see is patching the package on install. We don't have the bandwidth to maintain multiple versions of the typings.

[amcasey]

@eps1lon Sorry, I think my question might have been unclear. I think CSSProperties is fine (though, obviously, if it would be faster if it were smaller) - I was actually hoping there might be room to simplify the subtypes in https://github.com/mui-org/material-ui/blob/master/packages/material-ui-styles/src/withStyles/withStyles.d.ts. For example, would you get fewer completions if you changed this type to any?

Edit: on my box, this makes compiling the docs project 15% faster (29.7s down to 25.5s), but I don't know the effect on the editing experience.
Edit 2: Actually, once you've given up on folding in the recursive part of the type, you can just use BaseCreateCSSProperties and any other property will have type any (i.e. you get to keep the real types of the CSS properties).
Edit 3: Edit 2 doesn't work because of excess property checking (i.e. arbitrary property names are not allowed in object literals).

Your point about completions vs type checking was super interesting because I had a hypothesis that some type authors might feel that way. @DanielRosenwasser I think we looked into doing something like this to accommodate the pattern "a" | "b" | string - did that go anywhere?

[amcasey]

Also note that styled-components is having (we believe) closely related checker performance issues.

[amcasey]

Regarding being able to specify the type more exactly, I've filed https://github.com/microsoft/TypeScript/issues/36782.

[amcasey]

Looks like emotion might be in the same boat.

[lostpebble]

I started adding Material UI (4.9.4) to my project today, and the slowdown is really so significant that I don't think I can even use it in my project. Just added a simple <Slider/> component, customized using withStyles().

We're talking going from instant TypeScript feedback in my IDE to what feels like 5-10 seconds at times (for parts of my code that aren't even interacting with Material UI now - its just a complete TypeScript slowdown in the file that uses the component). Something must be significantly wrong with these types (or yea, overly complex), seems like @amcasey is doing some good investigations - I hope you can get to the bottom of it!

Trying to find a way that I can at least exclude all TypeScript stuff for @material-ui for now (basically make the entire module any) - but TypeScript doesn't seem to make that easy enough.

[oliviertassinari]

@lostpebble Does the same happen by using something else than withStyles to customize the slider, say CSS modules?

[amcasey]

@lostpebble We don't currently have a supported way to exclude a particular set of types. If you really, really want to, the thing to experiment with would be path mappings. You could try a path mapping like "@material-ui/*": ["simplemui"] and then create a simplemui.d.ts containing

declare const x: any;
export = x;

That would effectively make all material-ui types any. It's definitely a hack and not something I can recommend, but it might unblock your editing experience.

[eps1lon]

I think we've made some good improvements (somewhere in the 30% area) but it looks like all we could do right now is make the typings looser.

This absolutely requires concrete proposals e.g. which wrong code would you be ok with to be accepted by the type checker. Then we would need to benchmark and see if this makes a meaningful dent.

You have to understand that purposely introduced regressions add a lot of work for maintainers since we have to explain this to library consumers and is quite stressful overall.

So this isn't a priority for me at the moment unless somebody has actionable intell. Otherwise I have to put too much time into very little return.

[ypresto]

In my case for makeStyles(theme => createStyles(...)), returning Record<ClassKey, any> from createStyles(...) almost ~halves~ (in my code and computer, about ~1200ms -> 750ms~ 1400ms → 1100ms) encodedSemanticClassifications-full: elapsed time shown in tsserver log (it looks not an expensive job, but perhaps waiting for type check to be completed).

export default function createStyles<ClassKey extends string, Props extends {}>(
  styles: StyleRules<Props, ClassKey>,
): Record<ClassKey, any>;

createStyles(...) checks style structure so we can skip type check for argument-type-of-massive-union of makeStyles vs return-type-of-massive-union of createStyles.

~(And commenting out entire makeStyles code: 650ms)~

[eps1lon]

@ypresto createStyles is only needed for typescript versions without const assertions. If you can use { display: 'block' as const } in your codebase (ts >= 3.4) then use that over createStyles.

[amcasey]

@eps1lon We noticed that and tried making the switch in docs but the results were unimpressive.

[ypresto]

@eps1lon with const and without createStyles, IntelliSense does not show context-aware candidates anymore :cry:

[amcasey]

@ypresto It should. Do you have an example code snippet?

[ypresto]

@amcasey

Adding as const to the outer object or the property effectively kills it. I don't want to place it on every property.

const useStyles = makeStyles(theme => ({
  root: {
    // no IntelliSense
  }
} as const))

Also without createStyles it has small limitation for string completion.

const useStyles = makeStyles(theme => ({
  root: {
    direction: '|', // no IntelliSense at | (works with createStyles)
    direction: | // IntelliSense works at |
  }
}))

[amcasey]

@ypresto By "kills it", do you mean "causes it to work the same way as createStyles did"?

It's a drag to put it everywhere, but no more of a drag than putting createStyles everywhere.

[ypresto]

@amacleay I meant kills IntelliSense :pray:.

[amcasey]

Sorry, I missed your comments. I'll give that a shot.

[ypresto]

I have no idea why but this is 1100ms → 750ms, interestingly:

 export const DropArea: React.FC<CardProps & {
   active?: boolean
   description: string
   icon?: React.ReactNode
-}> = ({ active, description, icon, children, ...props }) => {
+}> = ({ children, ...props }) => {
   const classes = useStyles()
+  const active = false
+  const icon: React.ReactNode = null
+  const description = ''
   return (
     <Card {...props} className={clsx(classes.root)} variant="outlined">

Removing CardProps & from FC is almost the same result. Perhaps it is because CardProps extends PaperProps, which extends the large HTMLAttributes.

UPDATE & IMPORTANT: It turned out that replacing CardProps with HTMLAttributes<HTMLDivElement> also reduces time (not measured).

[ypresto]

Finally, I found the biggest one, 750ms → 130ms:

Removing style={...} from two Typographys.

-<Typography variant="subtitle2" component="div" noWrap style={{ width: '26ch' }}>...</Typography>
+<Typography variant="subtitle2" component="div" noWrap>...</Typography>
-<Typography variant="caption" component="div" noWrap style={{ width: '20ch' }}>...</Typography>
+<Typography variant="caption" component="div" noWrap>...</Typography>

But why? Adding the same style to <div> does not hit the performance. Maybe OverridableComponent is too complicated..?

(I'm using TypeScript 3.8.3, @material-ui/core 4.9.1)

[embeddedt]

AFAIK the way Material-UI handles local styles on components is different than the way React handles it for HTML elements.

[ypresto]

@embeddedt In type level, it resolves to React.CSSProperties which is the same as div's style prop.

[embeddedt]

@ypresto I stand corrected. Sorry.

[eric-burel]

Hi guys, not sure if it's worth opening a new issue for this, so I'll post it here since it's related to types correctness/performance. Let me know if I should open an issue instead.
When following the documentation to add a custom font, I end up with the following typing error:
Type 'string' is not assignable to type 'FontFaceFontDisplayProperty'

It's weird, because csstype 2.6.9 typings seems valid and other attributes are ok (using MUI 4.9.5).

const sourceSansPro = {
  fontFamily: "'Source Sans Pro'",
  fontStyle: "normal",
  fontDisplay: "swap", // won't work
  fontWeight: 400,
  src: `
    url('/static/fonts/Source_Sans_Pro/SourceSansPro-Regular.ttf') format("truetype")
  `
};

Theme property:

  overrides: {
    MuiCssBaseline: {
      "@global": {
        "@font-face": [sourceSansPro]
      }
    }

Type is type FontFaceFontDisplayProperty = "auto" | "block" | "fallback" | "optional" | "swap";

[eps1lon]

@eric-burel This is an isssue with typescripts' automatic type widening. Try

- fontDisplay: "swap", // won't work
+ fontDisplay: "swap" as "swap",

[eric-burel]

Thanks it works, and I've learnt a new concept "type widening" :) It's weird that fontStyle is not affected for example, it's not the only CSS property defined as an enum but the first time I hit this.

Edit: ok my bad it is well documented: https://material-ui.com/guides/typescript/#using-createstyles-to-defeat-type-widening

[yatrix7]

Finally, I found the biggest one, 750ms → 130ms:

Removing style={...} from two Typographys.

-<Typography variant="subtitle2" component="div" noWrap style={{ width: '26ch' }}>...</Typography>
+<Typography variant="subtitle2" component="div" noWrap>...</Typography>
-<Typography variant="caption" component="div" noWrap style={{ width: '20ch' }}>...</Typography>
+<Typography variant="caption" component="div" noWrap>...</Typography>

But why? Adding the same style to <div> does not hit the performance. Maybe OverridableComponent is too complicated..?

(I'm using TypeScript 3.8.3, @material-ui/core 4.9.1)

Have you found this to affect build time, or just the time it takes for intellisense to be useful? I'm getting the build issue (out of memory) and some of our TS code has a ton of style={someStyle} set on the components. Wondering if that's part of our issue.

[amcasey]

@yatrix7, generally speaking, I'd expect these long check times to affect both build and editor response times.

[FabianSellmann]

I anyone currently looking into this? I know there have been some improvements in regards to typescript type checking time in previous version upgrades. However, it is still slow.
Would not mind looking into this myself.

[eps1lon]

Would not mind looking into this myself.

That'd be awesome. We're currently not aware of actionable items for us to work on. So any pointers to bottlenecks would be appreciated.

[FabianSellmann]

Added some benchmarking for now, to help investigate: https://github.com/mui-org/material-ui/pull/22110

[amcasey]

@FabianKielmann On the TS end, I've been working on perf investigation tools that I hope will soon be mature enough apply to material-ui.

If you have some time to spend on this, you might also try something like https://github.com/microsoft/TypeScript/issues/38583.