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

Function return not being type checked within function body #29390

Closed
milesj opened this issue Jan 12, 2019 · 10 comments
Closed

Function return not being type checked within function body #29390

milesj opened this issue Jan 12, 2019 · 10 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Duplicate An existing issue was already created Fix Available A PR has been opened for this issue

Comments

@milesj
Copy link

milesj commented Jan 12, 2019

TypeScript Version: 3.2.2 and 3.3.0-dev.20190111

Search Terms: function return, function return type, function return not type checking

Code

I'm writing a type-safe React CSS-in-JS library called Aesthetic. For the most part, the library is complete and works great. The biggest issue is that TS is not type checking correctly on the consumer side. Before I dig into an example, a little background on the type system.

Aesthetic defines a handful of types (based on csstype) to structure what the "CSS object" can look like. This includes properties, attribute selectors, pseudo selectors, at-rules, and other nested values. These can all be seen here: https://github.com/milesj/aesthetic/blob/master/packages/core/src/types.ts#L40

Styles are defined for React components using an HOC approach, by calling a function on the Aesthetic instance: aesthetic.withStyles()(Component). The first argument to withStyles is a function that returns the "CSS object" or null to represent "no styles but give me the theme object".

The HOC factory is defined here: https://github.com/milesj/aesthetic/blob/master/packages/core/src/Aesthetic.tsx#L271
The style function type is defined here: https://github.com/milesj/aesthetic/blob/master/packages/core/src/types.ts#L78
An example of this in action:

function Button({ children, styles }: Props & WithStylesProps) {
  return (
    <button type="button" className={classes(styles.button)}>
      {children}
    </button>
  );
}

export default withStyles(({ unit }) => ({
  button: {
    textAlign: 'center',
    display: 'inline-block',
    padding: unit,
  },
}))(Button);

The issue is the function's returned object in the withStyles 1st argument is not being type checked, so all usability and development experience is thrown out the window right now. I put together a test case showing this behavior: https://github.com/milesj/aesthetic/blob/master/packages/core/tests/properties.tsx

And a few screenshots showing the behavior in VS code.

When using objects directly (not as a function return), the invalid properties are correctly highlighted.

screen shot 2019-01-11 at 18 47 59

When using a function, the invalid properties are not highlighted.

screen shot 2019-01-11 at 18 48 53

What's even more confusing is the VS code tooltips display the correct type for the argument, StyleSheetDefinition.

screen shot 2019-01-11 at 18 51 07

I've also tried removing the null union from StyleSheetDefinition, export type StyleSheetDefinition<Theme, Props = any> = (theme: Theme, props: Props) => StyleSheet;, but the same problem still persists.

Expected behavior:

Objects returned in a function are typed according to their contract.

Actual behavior:

Objects returned in a function are not being typed.

Playground Link: http://www.typescriptlang.org/play/#src=type%20StyleBlock%20%3D%20%7B%20display%3F%3A%20string%20%7D%3B%0D%0Atype%20StyleDefinition%20%3D%20(()%20%3D%3E%20StyleBlock)%20%7C%20null%3B%0D%0A%0D%0Afunction%20withStyles(func%3A%20StyleDefinition)%20%7B%0D%0A%20%20return%20(component%3A%20Function)%20%3D%3E%20null%3B%0D%0A%7D%0D%0A%0D%0Afunction%20Component()%20%7B%0D%0A%20%20return%20null%3B%0D%0A%7D%0D%0A%0D%0Aconst%20StyledComp%20%3D%20withStyles(()%20%3D%3E%20(%7B%0D%0A%20%20display%3A%20'foo'%2C%0D%0A%20%20invalid%3A%20true%2C%0D%0A%7D))(Component)%3B

Related Issues: Couldn't find any applicable.

@weswigham
Copy link
Member

So, technically, a {display: string, invalid: boolean} is assignable to a {display?: string} which is why it typechecks. Most of the time you'd expect our excess property checking to take effect and require an exact type, but because this is the return value of a callback, function return type widening prevents excess property checking from ever taking place.

Root cause is #241 😄

@weswigham weswigham added Duplicate An existing issue was already created Design Limitation Constraints of the existing architecture prevent this from being fixed labels Jan 14, 2019
@milesj
Copy link
Author

milesj commented Jan 14, 2019

@weswigham Is this something that will ever work? If not, do you suggest an alternate API?

@weswigham
Copy link
Member

Not really, no 😦 #241 is a long-documented drawback to how function expression returns are handled and why it's often not desired.

@milesj
Copy link
Author

milesj commented Jan 14, 2019

Well that's just unfortunate 😭

@weswigham
Copy link
Member

Yeah, I guess that's was way to expressly forbid excess properties by using inference.

@milesj
Copy link
Author

milesj commented Jan 16, 2019

Sweet, I'll give that a shot and report back.

@milesj
Copy link
Author

milesj commented Jan 17, 2019

Tried to never this stuff but the types are so complicated I've yet to get it to type the entire structure all the way down.

@milesj
Copy link
Author

milesj commented Jan 19, 2019

Here's the PR: aesthetic-suite/framework#41

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Duplicate An existing issue was already created Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants