[system] style option "transform" has invalid typing

[akomm]

The typing for the style function only allows transform to number or string. However also objects should be allowed, as you can see in system's package own code here

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Should not fail type check:

const verticallySpaced = style({
  prop: 'vSpaced',
  cssProperty: false,
  transform: value => ({
    '& > :not(:last-child)': {
      marginBottom: value,
    },
  }),
});

Current Behavior 😯

Fails with:

Type '(value: unknown) => { '& > :not(:last-child)': { marginBottom: unknown; }; }' is not assignable to type '(cssValue: unknown) => ReactText'.
  Type '{ '& > :not(:last-child)': { marginBottom: unknown; }; }' is not assignable to type 'ReactText'.
    Type '{ '& > :not(:last-child)': { marginBottom: unknown; }; }' is not assignable to type 'number'.

Steps to Reproduce 🕹

See expected behavior example.
update Example: https://codesandbox.io/s/busy-joliot-qfkmh

Context 🔦

I think self-explaining by expected behavior.

Your Environment 🌎

Tech	Version
Material-UI	v4.1.3
React	16.8.3
TypeScript	3.5.2

[eps1lon]

Could you prepare a codesandbox to play around with? This would help alot in verifying that this is valid at runtime.

[akomm]

Yes here we go :)

https://codesandbox.io/s/busy-joliot-qfkmh

Also note, the way the cssValue in the transform function is specified does not allow for better typing when passing the transform function as StyleOptions. Should it not be instead a generic? I know unknown is substituted for not yet inferrable or known types, which might get inferred later. But never seen usage of explicit unknown as type hint.

Being able to spec the cssValue type is important, because it is inferred for the prop value on the enhanced component! :)

Maybe it is also possible to allow the object return from transform when cssProperty is false? Probably would require union and not sure if that hits some of the limitations typescript currently has in this regards.

[akomm]

@eps1lon I found additional errors in the same typing, so related to this issue. I have changed the transform in the following examples to make this other issues with the typing visible.

Example: https://codesandbox.io/s/nifty-varahamihira-o13xh
Issue: the vSpaced property is required. It should always be optional.

Example: https://codesandbox.io/s/boring-hawking-qmtj9
Issue: the theme property (should come via context from the theme provider?) is missing.

I also noticed some weirdness:

export interface StyleOptions<PropKey, Theme extends object> {
  cssProperty?: PropKey | keyof CSS.Properties | false;
  prop: PropKey;
  /**
   * dot access in `Theme`
   */
  themeKey?: string;
  transform?: (cssValue: unknown) => number | string;
}

The Theme generic is not used.

[akomm]

@eps1lon

The longer I am on the thing, the more I realize its utterly broken. And that not only on the typing side.

I am working on a fix for me and found an implementation error in the style function.

Given this monkey patch:

import * as CSS from 'csstype';
import {style as brokenStyleFunction} from '@material-ui/system';

type FixedStyleOptions<PropKey extends string, PropType> = {
  cssProperty?: PropKey | keyof CSS.Properties | false,
  prop: PropKey,
  themeKey?: string,
  transform?: (cssValue: PropType) => number | string | {[K: string]: any},
}
type FixedStyleFunctionFactory =
  <PropKey extends string, PropType>(options: FixedStyleOptions<PropKey, PropType>)
    => (props: {[K in PropKey]?: PropType})
      => any;

const style: FixedStyleFunctionFactory = brokenStyleFunction as any;

export default style;

Which gave me:

• the style prop is optional
• the style prop type is inferred correctly
• possibility to transform to more complex style (media-query, child selector, etc.)

Note: this is not complete, I am about to figure out the point of the theme prop added in, because normally theme comes from context of component which is styled and no use-case is documented (or I missed it) where theme is passed as option to style. Hence when used as documented you will have to pass theme to every component or pass theme into your style function when you create it...

However, before I could figure it out further, I encoutered a runtime implementation error in the style function.
Returning null is invalid here.

I think JSS styled does not expect a null there.

[akomm]

@eps1lon or someone feels also responsible for it:

Is there anything I can do further to make it easier for you so you can confirm whether it is intended or not intended to work like that, or whether my points are valid or not, so further thoughts about PR can me made? I won't start a PR until I know there is no special Idea behind all or some of the things I see as unexpected behavior. I kind of see that you maybe want some of the stuff as a separate issue, that I found out afterwards, but they seem to be the same issue to me.

Any mechanics like tagging as "bug" or "confirmed"? or "wontfix"? Thanks in advance ! :)

[eps1lon]

Yeah the best course of action for typescript issues is to actually open a PR with your proposed typings as this will run all the tests. Very often people propose solutions that only helps their problem but introduces unsound types or breaks other typing patterns. Having a PR with all the tests and concrete code patches helps a lot.

[akomm]

@eps1lon

I am working on a PR. I would like to sum up the issues:

1. style-factory: transform-typing is wrong and does not allow complex projection (media-query, child-selector, etc.).
2. style-factory: using themeKey and transform does not work together. I wrote test changing this and allowing both to be used. IT does not break existing tests. It allows to project complex styling still using theme-mapping, including breakpoints.
3. style-factory **1: created function does return null, when property value is null (non-strict comparison). This however breaks JSS, which get null at some point, along the path style -> styled -> makeStyles. Returning {} instead fixes the issue an seem test-wise BC.
4. style-factory: transform-function value typing is unknown and does not allow type-declaration and also does not allow type-inference for enhanced components.
5. style-factory: typing for created style function makes the style property on the enhanced component required.
6. styled-factory: typing for created style function makes the styled-enhanced components require theme property. I think this is wrong to assume, what if theme is provided via ThemeProvider? **2

**1 Who is doing it wrong, styled not handling null, or style returning null? Question is important, as I would like to not make PR crossing two different packages - if possible
**2 Need feedback on whether I am correct about this assumption and Theme requirement should be dropped

In my opinion, the PR should include all the fixes because every issue makes the other fix kind of pointless I ralized. Do you agree?

update added links

[eps1lon]

You can put this all into a single PR. It would help if you keep changes as atomic as possible i.e. if you can isolate changes please do so via multiple commits in case it would help break it up into PRs after some discussion. For example it seems like the unknown removal isn't necessary for the other changes

as I would like to not make PR crossing two different packages

The thing about monorepos is to allow exactly that. Don't bother too much about crossing package boundaries.

[akomm]

Got all the stuff fixed and passing tests (including new tests).

Now about to break it up like "add fail test for X" "fix X" as you proposed. Kind of hard if you can not simply reference separate issues ;)

[akomm]

@eps1lon so the style factory is not tested at all type-wise (index.spec.tsx). The moment I simply introduce a test and call style it does not work. And whenever I add a fix, it still not work until I add them all. Its like a closed chain. This includes the unknown which you said can be separated.

[eps1lon]

The moment I simply introduce a test and call style it does not work.

Can you open a draft PR so I can review what's not working? I don't know what "does not work" means.

[akomm]

I was talking about the index.spec.tsx, containing tests for the index.d.ts (mui-system). So what is not working is that for usage with styled-component and styled from mui-system fail differently and need different amount of fixed to be fixed. Both work correctly type-wise, when everything is fixed. Simply put: the style function is not tested in the index.d.ts spec file at all, and once you introduce it in some way and invoke, its hard to isolate single issues if you want to test it both against mui styled and styled-components. Simply open a WIP PR is what I have planed, however its not as simple as it sounds in terms of isolation. So I need to make a test for all the typing issues and then fix for all the typing issues. The transform+themeKey thing can probably be isolated though.

@eps1lon

So you see, this simple example: I had to for example here to add theme={{}} to hide away the theme issue. The other issues would require further temporary quirks added, before the final fixes actually fix it and I can revert. Kind of weird... When I continue fixing theme, etc, etc in a single fix, all issues are fixed until no such quirks are required. xD