test: Investigate removing IsEmptyInterface #1
Conversation
__snapshots__/IsEmptyInterface
Outdated
| Emit time: 0.00s | ||
| Total time: 4.12s | ||
| Total time: 5.37s |
There was a problem hiding this comment.
It's just a single run. When averaging 10 runs it did get slightly faster (6%). The snapshot is more for the other diagnostics (see snapshots/README.md)
| * {} is a top type so e.g. `string extends {}` but not `string extends object` | ||
| * 3. false if the given type is `unknown` | ||
| */ | ||
| export type IsEmptyInterface<T> = And< |
There was a problem hiding this comment.
I was expecting CoerceEmptyInterface and maybe IsAny, And, and Or to go as well. Are they used for other things?
There was a problem hiding this comment.
We can remove CoerceEmptyInterface. The others are used to check if we can call useStyles with one or zero arguments.
I'll cherry-pick mui/material-ui#19243 and mui/material-ui#19259 first and see if we get a more significant improvement.
There was a problem hiding this comment.
Hard to tell since these were only affecting withStyles and styled. We did get some slight runtime improvements in our test_types job but these are only single runs. Might not have been significant.
Having a large codebase where I can just run tsc on would help. The original repo in microsoft/TypeScript#34801 didn't help much because it includes a full webpack build (and therefore noise).
My guess is that types related to CSSProperties are the issue. These are quite complicated so I didn't look into them much. Will be todays and probably tomorrows agenda since today is a bit busier than usual.
There was a problem hiding this comment.
There were some repro notes in microsoft/TypeScript#34801. Would they get you more useful numbers?
There was a problem hiding this comment.
All I see is "takes forever to report errors in vscode". Without know their vscode environment or how long "forever" is I can't do much. Sorry but I can't find detailed instructions.
There was a problem hiding this comment.
I'm sure I could reconstruct a repro, but I think this is a bit of a tangent anyway - my local experiments suggest that the CSS properties types are more impactful.
import { makeStyles } from "@material-ui/styles";
// https://github.com/mui-org/material-ui/issues/19113#issuecomment-573614279
const useStaticStyles = makeStyles({ root: { color: "blue" } });
const useDynamicStyles = makeStyles({
root: { color: (props: { color: string }) => props.color }
});
function Component() {
const staticClasses = useStaticStyles(); // No error
const throwingClasses = useDynamicStyles(); // $ExpectError
const dynamicClasses = useDynamicStyles({ color: "blue" });
}-- https://github.com/eps1lon/mui-types-perf/blob/master/sources/IsEmptyInterface.test.ts |
eps1lon
force-pushed
the
test/always-optional-props
branch
from
40d1afb
to
53af345
Compare
Removes
IsEmptyInterface. There are slight reductions in the number of Nodes, Identifiers, Symbols and Types butyarn perf:IsEmptyInterfacedoes not yield a significant improvement:4.52478006190000000000 seconds per run seconds per runon master compared to4.27613066370000000000 seconds per runon this branch0.2486493982reduction or 6%.Maybe we need to improve other areas first to see a significant improvement (e.g. BaseCSSProperties). Maybe we need a larger codebase to check.
/cc @amcasey