Skip to content

test: Investigate removing IsEmptyInterface #1

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

test: Investigate removing IsEmptyInterface #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c697b7
test: Investigate removing IsEmptyInterface
eps1lon Jan 15, 2020
53af345
Remove IsAny, Or, And
eps1lon Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions __snapshots__/IsEmptyInterface
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
sources/IsEmptyInterface.test.ts(11,27): error TS2554: Expected 1 arguments, but got 0.
Files: 54
Lines: 61523
Nodes: 180439
Identifiers: 62593
Symbols: 104001
Types: 36907
Memory used: 133717K
Lines: 61464
Nodes: 180242
Identifiers: 62543
Symbols: 103216
Types: 36751
Memory used: 152222K
I/O read: 0.02s
I/O write: 0.00s
Parse time: 0.68s
Bind time: 0.76s
Check time: 3.13s
Parse time: 0.62s
Bind time: 0.24s
Check time: 3.11s
Emit time: 0.00s
Total time: 4.57s
Total time: 3.97s
23 changes: 2 additions & 21 deletions vendor/@material-ui/styles/makeStyles/makeStyles.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,9 @@ import {
Styles,
WithStylesOptions,
} from '@material-ui/styles/withStyles';
import { Omit, IsAny, Or, IsEmptyInterface } from '@material-ui/types';
import { Omit } from '@material-ui/types';
import { DefaultTheme } from '../defaultTheme';

/**
* @internal
*
* If a style callback is given with `theme => stylesOfTheme` then typescript
* infers `Props` to `any`.
* If a static object is given with { ...members } then typescript infers `Props`
* to `{}`.
*
* So we require no props in `useStyles` if `Props` in `makeStyles(styles)` is
* inferred to either `any` or `{}`
*/
export type StylesRequireProps<S> = Or<
IsAny<PropsOfStyles<S>>,
IsEmptyInterface<PropsOfStyles<S>>
> extends true
? false
: true;

/**
* @internal
Expand All @@ -33,9 +16,7 @@ export type StylesRequireProps<S> = Or<
* from which the typechecker could infer a type so it falls back to `any`.
* See the test cases for examples and implications of explicit `any` annotation
*/
export type StylesHook<S extends Styles<any, any>> = StylesRequireProps<S> extends false
? (props?: any) => ClassNameMap<ClassKeyOfStyles<S>>
: (props: PropsOfStyles<S>) => ClassNameMap<ClassKeyOfStyles<S>>;
export type StylesHook<S extends Styles<any, any>> = (props?: PropsOfStyles<S>) => ClassNameMap<ClassKeyOfStyles<S>>

export default function makeStyles<
Theme = DefaultTheme,
Expand Down
6 changes: 2 additions & 4 deletions vendor/@material-ui/styles/withStyles/withStyles.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { PropInjector, IsEmptyInterface } from '@material-ui/types';
import { PropInjector } from '@material-ui/types';
import * as CSS from 'csstype';
import * as JSS from 'jss';
import { DefaultTheme } from '../defaultTheme';
Expand Down Expand Up @@ -43,9 +43,7 @@ export interface CreateCSSProperties<Props extends object = {}>
*/
export type StyleRules<Props extends object = {}, ClassKey extends string = string> = Record<
ClassKey,
IsEmptyInterface<Props> extends true
? CSSProperties | (() => CSSProperties)
: CreateCSSProperties<Props> | ((props: Props) => CreateCSSProperties<Props>)
CreateCSSProperties<Props> | ((props: Props) => CreateCSSProperties<Props>)
>;

/**
Expand Down
38 changes: 0 additions & 38 deletions vendor/@material-ui/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,41 +44,3 @@ export type Omit<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof
* @internal
*/
export type Overwrite<T, U> = Omit<T, keyof U> & U;

/**
* Returns true if T is any, otherwise false
*/
// https://stackoverflow.com/a/49928360/3406963 without generic branch types
export type IsAny<T> = 0 extends (1 & T) ? true : false;

export type Or<A, B, C = false> = A extends true
? true
: B extends true
? true
: C extends true
? true
: false;

export type And<A, B, C = true> = A extends true
? B extends true
? C extends true
? true
: false
: false
: false;

/**
* @internal
*
* check if a type is `{}`
*
* 1. false if the given type has any members
* 2. false if the type is `object` which is the only other type with no members
* {} 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<
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting CoerceEmptyInterface and maybe IsAny, And, and Or to go as well. Are they used for other things?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the other commits help?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were some repro notes in microsoft/TypeScript#34801. Would they get you more useful numbers?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keyof T extends never ? true : false,
string extends T ? true : false,
unknown extends T ? false : true
>;