[POC] Styled render callback component and refactored withStyles #9503

rosskevin · 2017-12-15T01:31:55Z

DO NOT MERGE, for review/feedback only

POC towards #9272 (comment)

related: #8542 #8447 #7636

Approach

Move the implementation of withStyles to Styled and retain withStyles API exactly as it has been.

Deficiencies

anything with the comment REVIEW
determining stylesheet names (will be impossible? without HOC)?
no attempt at TS types yet
indexCounter could be irrelevant now?

Usage

current withStyles HOC or new render callback:

<Styled styles={myStyles}>
  {(classes, theme) => <div className={classes.root} />}
</Styled>

kof · 2017-12-15T01:40:59Z

src/styles/Styled.js

+    }
+
+    warning(
+      indexCounter < 0,


This warning isn't that useful. It' in case you called the component many many times. But would have probably found the issue before.

Still, the warning doesn't harm.

oliviertassinari

This is an API people have been asking for different time in the past. I like the flexibility compound component provides. I would vote for keeping both API. I have been giving some thought on the tradeoff both API offer on an old issue

On a side note. We need to be cautious with this new API. I think that we should keep using the HOC API. I don't want newcomers to ask themselves which API they should be using. What's the best one? This is unneeded complexity for newcomers. It doesn't matter for them. Just use one and stick to it.

oliviertassinari · 2017-12-15T07:16:30Z

prettier.config.js

@@ -1,11 +0,0 @@
-// @flow
-
-module.exports = {


Why? Isn't it better to have all our configuration in js?

Easier to use yml, it's text only. Shorter file name. Dot file name, more hidden in the src tree.

My order of preference for configuration files is: js > yml > json. But as you prefer :).

oliviertassinari · 2017-12-15T07:17:48Z

src/styles/Styled.js

+    }
+
+    warning(
+      indexCounter < 0,


This warning isn't that useful. It' in case you called the component many many times. But would have probably found the issue before.

oliviertassinari · 2017-12-15T07:18:43Z

src/styles/withStyles.js

-      return <Component classes={classes} {...more} {...other} ref={innerRef} />;
-    }
-  }
-
  Style.propTypes = {
    /**


Do we need this?

I mean, the propTypes ?

innerRef is exclusive to withStyles, classes though can be omitted. --Actually, I don't think we need any (including innerRef), just pass-through now.

oliviertassinari · 2017-12-15T07:20:22Z

src/styles/withStyles.js

-      'Material-UI: you might have a memory leak.',
-      'The indexCounter is not supposed to grow that much.',
-    ].join(' '),
+  const Style = () => (


We almost never use anonymous arrow function for the components definitions.

Ok, why not? (curious) What do you prefer here?

oliviertassinari · 2017-12-15T07:21:50Z

src/styles/Styled.js

+}
+
+class Styled extends React.Component {
+  static defaultProps = {


I have been removing all the static properties as they get in the way of refactoring components from classes to functions.

Don't do that if it is still a class, it is invalid typescript and could harm our potential conversion. If it's a class, static props are the right way, the other way is just a hack after all.

Oh, TypeScript doesn't support this?

You have to: augment the definition of the target, or cast it. So it supports it, but you have to cast it to the type first. In other words it is uglier on the outside, vs static class prop.

Alright. At least I can keep using this pattern at work. It's saving us time refactoring component class <-> function.

oliviertassinari · 2017-12-15T07:22:44Z

src/styles/Styled.js

+    }
+
+    warning(
+      indexCounter < 0,


Still, the warning doesn't harm.

…gistry

rosskevin · 2017-12-15T20:47:08Z

@kof @oliviertassinari perhaps you might have a thought on this one.

When using Styled - styles is passed as a prop, and is not the same object as const styles in the original component. This causes a sheet to be not found, and a new one created on every render. Specifically:

    let sheetManager = this.sheetsManager.get(this.stylesCreatorSaved);

    if (!sheetManager) {
      sheetManager = new Map();
      this.sheetsManager.set(this.stylesCreatorSaved, sheetManager);
    }

where this.stylesCreatorSaved is withStyles(styles)/<Styled styles={styles}/> sourced from const styles in the component file. Before when styles was passed in as an option to the HOC, this was not a problem because styles === this.stylesCreatorSaved. It appears react passes a copy.

This is a show stopper if not solved because it causes massive sheet growth.

^^^Next question: If this can be solved, won't we still have the same growth issue with interpolating props into styles?

If you want to run it - look at withStyles.spec.js, specifically: should provide a classes property and debug Styled:160

rosskevin · 2017-12-15T22:00:03Z

I’m wrong about the props being modified - I committed a hack that proves it. It is this.stylesCreator = getStylesCreator(props.styles)

But, my comment about sheet growth with props interpolation is something to be weary of with this approach.

Moving forward again...

rosskevin · 2017-12-15T22:26:11Z

So it appears that the result of:
getStylesCreator(styles) attempt 1 !== getStylesCreator(styles) attempt 2
even though
styles attempt 1 === styles attempt 2

rosskevin · 2017-12-15T22:41:04Z

I think this may have been the source of the need for the hot loader hack. Maybe... just speculating at this point.

…es input. This is important - because it is the source of the sheet leak - the result of this is used as the key for the sheet manager

…ranoid

rosskevin · 2017-12-15T23:58:58Z

Targeting the withStyles.spec essentially as-it-was, only HMR with same state is failing and while I took a shot at it, I have to admit I don't understand it. I'm going to try some in browser tests and see if the parent-child order is a problem.

As @oliviertassinari said in chat:

@rosskevin I have just realized one thing. I don't think it's technically possible to solve the CSS priority issue with the Styled architecture and index based counting approach. Maybe I'm missing something. I'm going to sleep. I will see clearer tomorrow.
Man, you just started a week+ task (I took me 3 weeks, 1 week to implement and 2 weeks to stabilize after user feedbacks, when revamping Nathan solution). This Styled API might require a rewrite.

…efore moving forward

rosskevin · 2017-12-16T01:04:40Z

Ok @oliviertassinari @pelotom - I've got to go for the night. I've got a full day into this (plus a little yesterday).

Two things need to be considered:

parent/child issue - my guess is that is what is at play in the docs AppBar as well as Menu:
spec changes - I have made Paper.spec work as a sample. A lot of repetition for getting to the component's first child. We can either create a utility for this or use some enzyme foo to make this easier, but I don't want to move on without a more readable solution (as well as to understand if we have the parent/child issue)

Any thoughts are welcome. I think this is a decision point - I need to confirm before spending more time.

rosskevin · 2017-12-16T01:10:56Z

src/styles/withStyles.js

+        }}
+      </Styled>
+    );
+  };


This is an example where the render callback pattern is better. withTheme and innerRef are HOC options not necessary using Styled. With a render callback pattern, the child chooses what it uses and what it ignores (e.g. theme), and the innerRef indirection is unnecessary because the parent can put the ref where ever it chooses. This code shows what it took to match the various current usage patterns of the HOC.

kof · 2017-12-16T09:29:12Z

We should switch to react-jss, really, maintaining both projects with edge cases bugs is hard. I know there are some things missing, we need to figure out how to add them.

rosskevin · 2017-12-16T13:28:39Z

We should switch to react-jss, really, maintaining both projects with edge cases bugs is hard. I know there are some things missing, we need to figure out how to add them.

@kof several thoughts come to mind:

I would have liked to have this comment 24 hours ago when I put in 1h and asked for a review from you and before I spent 9h yesterday! Nonetheless, I think I'm up to speed on our styling solution now and I know you are busy yourself.
I don't know the reasons why we have ours vs react-jss - and I would like to understand the details now that I am thoroughly familiar with our solution
react-jss doesn't seem to offer a render callback component, which this PR (and linked issues) is targeting.

A cursory review of react-jss seems to share many similarities, but the devil is in the details.

Is there any reason for us to continue with our mui solution vs react-jss?
Is there a way with react-jss to get a render callback pattern and solve the linked issues concerns?

kof · 2017-12-16T14:06:36Z

@rosskevin

We talked with @oliviertassinari many times and he created a bunch of issues on react-jss in order to move most parts of the logic at some point (most of them have been already resolved and released). In order to move faster, it is easier to implement things in mui directly first and then find a way to add them to react-jss if they make sense for more general use cases. MUI might still have very specific requirements, where react-jss can't or shohuldn't cover them.

React-JSS has a render callback interface issue for a long time now, it was never clear why we would want that, it still isn't, but it is certainly an interesting interface.

I am always hesitating to introduce an alternative interface, because it will make users to choose, without having a very clear explanation of "why".

rosskevin · 2017-12-16T14:50:39Z

/cc @oliviertassinari - I forgot that I'm so far down the road of static typing, that others might not recognize the pain of HOCs. So @oliviertassinari, some of this is I owe to you based on our chat yesterday.

Users can choose one or the other, where it suits their use case. In my code (for styles), I would use the render callback pattern exclusively because styling is a render-only use case - in other words - none of the props injected have any bearing on the rest of the implementation of the components as they are only needed to render (e.g. not needed in componentWill*). I also have enough other HOCs (in app code) where adding another is just painful (especially with tying together a strongly typed component).

Render callbacks are significantly less static type coupling and coupling in general:

no merging of external facing fabricated component props (e.g. hoc optional props + component props)
no requirement of direct child to implement injected props (hoc requires child component to accept all injected props - then omit them from it's children)
freedom of render structure (e.g. insert or reorder children, the top level doesn't have to accept injected props anymore)
explicit clarity of source of props (no magic wondering where the prop is coming from)
debugging is easier
static type errors are dramatically easier

I need no more convincing that this pattern is dramatically easier to create, maintain, and use. BUT, with that said, there are cases (probably 10%) where a HOC is a nicer pattern of use (albeit not a better pattern to have to maintain). Since any render callback pattern can also be easily wrapped and exposed as a HOC, it seems like a no-brainer to do.

kof · 2017-12-16T14:56:45Z

@rosskevin yeah, sounds very convincing as you say it, worth a blog post with all the details! Now I am actually more motivated to add it to React-JSS.

rosskevin · 2017-12-16T15:00:23Z

So @kof - is react-jss going to implement it - or do we just continue here on our own?

kof · 2017-12-16T15:02:24Z

Would you like to try add it to react-jss? I haven't thought much about technical implications or challenges yet, but it would be certainly worth trying.

Bnaya · 2017-12-16T15:39:37Z

A Hint,
There's a pitfall with mobx and this pattern.

const MyComponent = observer(({ model }: { model: MyMobxModel }) => {
  return (
    <Styled styles={myStyles}>
      {({ classes }) => <div className={classes.root}>{model.name}</div>}
    </Styled>
  );
});

The function inside the Styled is actually another component, which is not observer, and will not render on model.name changes.
So you will need to do something like:

const MyComponent = observer(({ model }: { model: MyMobxModel }) => {
  return (
    <Styled styles={myStyles}>
      {observer(({ classes }) => <div className={classes.root}>{model.name}</div>)}
    </Styled>
  );
});

But i'm not sure if this will work

Edit - all of the above no relevant, my mistake,
You can use mobx observer also as component. so you just need to nest it inside the Styled

<Styled styles={myStyles}>
  {({ classes }) => (
    <Observer>
      {() => <div className={classes.root}>{model.name}</div>}
    </Observer>
  )}
</Styled>;

mweststrate · 2017-12-17T14:51:20Z

@Bnaya note that Observer should be uppercased to use it as component

pelotom

I just want to note here that unfortunately I don't think Styled will be very usable in TypeScript until microsoft/TypeScript#6395 is solved. The reason is that the Styled component needs to be generic in the keys of its style prop. IOW, you want to give it a type like

interface StyledProps<ClassKey extends string> {
  styles: Record<ClassKey, React.CSSProperties>;
  children(classes: WithStyles<ClassKey>): JSX.Element;
}

class Styled<ClassKey> extends React.Component<StyledProps<ClassKey>> {
  // ...
}

but then when you go to use it there's no easy way to instantiate the generic parameter, and type inference won't help you:

// doesn't work--`ClassKey` can't be inferred
<Styled styles={myStyles}>
  {classes => /* ... */}
</Styled>

// also doesn't work--syntax error
<Styled<keyof myStyles> styles={myStyles}>
  {classes => /* ... */}
</Styled>

rosskevin · 2017-12-20T00:53:15Z

I'm closing this out (for now):

We have a bit of a typescript pain that is not resolvable yet
This shows the parent/child order problem
We'd like to utilize react-jss as much as possible (this doesn't help there)

I'm still firmly for using this pattern, but I'm not sure the best place to do it is here vs react-jss, as well as how it impacts types (which I really like the validation as it sits today).

oliviertassinari · 2017-12-20T08:10:14Z

@rosskevin Thanks for working on the POC. The conclusions are great starting points for anyone who would like to push the effort forward!

Bnaya · 2018-01-09T09:46:11Z

interface StyledProps<ClassKey extends string> {
  children(classes: WithStyles<ClassKey>): JSX.Element;
}


function createStyled<ClassKey extends string>(stylesParam: Record<ClassKey, React.CSSProperties>) {
  return class InnerStyled extends React.Component<StyledProps<ClassKey>> {

  }
}

const Styled = createStyled({elementA: {}, e2: {}});
const b = <Styled>
  {(styles) => {
    return <div className={styles.classes.elementA}><a className={styles.classes.e2}></a></div>
  }}
</Styled>;

@rosskevin The above pattern will work type safely also now.
You can also make createStyled to take a function with the theme lazily

Bnaya · 2018-01-09T15:08:07Z

@oliviertassinari @rosskevin

I've made this function, would like to hear your feedback

import { WithStyles } from "material-ui";
import { Theme } from "material-ui/styles";
import withStyles from "material-ui/styles/withStyles";
import * as React from "react";

interface IStyledProps<ClassKey extends string> {
  children(classes: WithStyles<ClassKey>): JSX.Element;
}

type stylesParamType<ClassKey extends string> = (
  theme: Theme,
) => Record<ClassKey, React.CSSProperties> | Record<ClassKey, React.CSSProperties>;

export function createStyled<ClassKey extends string>(stylesParam: stylesParamType<ClassKey>) {
  const wrapperComponent = withStyles(stylesParam);

  const createdStyled: React.StatelessComponent<IStyledProps<ClassKey>> = function createdStyledComponent(props) {
    return React.createElement(
      wrapperComponent(stylePropsFromWithStyles => {
        return props.children(stylePropsFromWithStyles);
      }),
    );
  };

  return createdStyled;
}

const styles = (theme: Theme) => ({
  root: {
    width: "100%",
    background: theme.palette.background.paper,
    flex: "1",
    overflowY: "scroll",
  } as React.CSSProperties,
});

const Styled = createStyled(styles);

const a = <Styled>{(styleLocal) => {
  return <div className={styleLocal.classes.root}>Test</div>;
}}</Styled>;

oliviertassinari · 2018-01-09T22:33:24Z

@Bnaya I have been implementing something close in https://gist.github.com/oliviertassinari/dceb407e99d7279cc4228957c068d27c. It's a styled-components like API.
I fear the CSS precedence issue is a blocker for pushing the compound API forward :/.

Bnaya · 2018-01-09T22:56:10Z

What's the CSS precedence issue?
My code also gives you correct typescript types for the class names

oliviertassinari · 2018-01-09T23:02:07Z

@Bnaya You can use this API outside of a React render method, you can't use it inside. If you do, the injected CSS won't get precedence on the children you are trying to customize.

Bnaya · 2018-01-09T23:14:54Z

Can you link to the relevant issue (if any)?

oliviertassinari · 2018-01-10T20:16:43Z

@Bnaya There is no "opened" issue for this, nor link as it's pure speculation from my experience. If you build the component outside of the render method, in a static way, it should be good :).
This means that we potentially have 3 different styling APIs. The implementation of the 2 other APIs is simple and short sugar on top of withStyles(). This is exciting. Thanks for sharing. It's something people need to be aware of. But I'm wondering. What's the best way to deliver?

rosskevin added 2 commits December 14, 2017 18:29

switch prettier config to yaml

1362466

prototype - Styled render callback and withStyles refactoring

a36895b

rosskevin requested review from oliviertassinari, pelotom, kof and nathanmarks December 15, 2017 01:32

kof reviewed Dec 15, 2017

View reviewed changes

oliviertassinari reviewed Dec 15, 2017

View reviewed changes

rosskevin added 3 commits December 15, 2017 11:19

fix getClasses helper to mount and obtain the styleSheet via sheetsRe…

f045ce6

…gistry

bad prop

73fbad0

rename dummy field class name to prevent confusion

bf74fdd

rosskevin added the on hold There is a blocker, we need to wait label Dec 15, 2017

I’m wrong about the props being modified - this proves it.

a56befa

hack showing difference causing sheet growth

0fe9008

rosskevin added 10 commits December 15, 2017 16:47

proof that getStylesCreator creates a different result with same styl…

8be0604

…es input. This is important - because it is the source of the sheet leak - the result of this is used as the key for the sheet manager

oops, NOW this proves it. sneaky.

52e2603

add broken styles function equality test

bb911be

fixed creator as key problem

f42d156

move the indexCounter into getStylesCreator - more relevant there.

c042d5c

fix passing of classes prop

caece9a

reestablish innerRef on withStyles

926db46

ensure different creators for different style objects - just being pa…

8ff4562

…ranoid

tests checkpoint

e4934e5

fix resolution of listenToTheme

7abdf6d

Add tests for withStyles (withTheme) and fix Styled

7717eba

rosskevin added 2 commits December 15, 2017 18:49

more fixes - first other spec green Paper.spec - should be reviewed b…

7060920

…efore moving forward

name should not be propagated

ff5a53d

rosskevin commented Dec 16, 2017

View reviewed changes

decouple props and establish render callback signature

a41f9e8

pelotom reviewed Dec 18, 2017

View reviewed changes

rosskevin closed this Dec 20, 2017

rosskevin mentioned this pull request Dec 22, 2017

v2.1 Implemented the Query Component apollographql/react-apollo#1398

Merged

rosskevin deleted the styled-withstyles branch January 25, 2019 16:35

[POC] Styled render callback component and refactored withStyles #9503

[POC] Styled render callback component and refactored withStyles #9503

Conversation

rosskevin commented Dec 15, 2017 • edited Loading

DO NOT MERGE, for review/feedback only

Approach

Deficiencies

Usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin commented Dec 15, 2017 • edited Loading

rosskevin commented Dec 15, 2017 • edited Loading

rosskevin commented Dec 15, 2017 • edited Loading

rosskevin commented Dec 15, 2017 • edited Loading

rosskevin commented Dec 15, 2017

rosskevin commented Dec 16, 2017

Choose a reason for hiding this comment

kof commented Dec 16, 2017 • edited Loading

rosskevin commented Dec 16, 2017

kof commented Dec 16, 2017 • edited Loading

rosskevin commented Dec 16, 2017

kof commented Dec 16, 2017 • edited Loading

rosskevin commented Dec 16, 2017

kof commented Dec 16, 2017

Bnaya commented Dec 16, 2017 • edited Loading

mweststrate commented Dec 17, 2017

pelotom left a comment • edited Loading

Choose a reason for hiding this comment

rosskevin commented Dec 20, 2017

oliviertassinari commented Dec 20, 2017

Bnaya commented Jan 9, 2018

Bnaya commented Jan 9, 2018

oliviertassinari commented Jan 9, 2018 • edited Loading

Bnaya commented Jan 9, 2018

oliviertassinari commented Jan 9, 2018 • edited Loading

Bnaya commented Jan 9, 2018

oliviertassinari commented Jan 10, 2018 • edited Loading

rosskevin commented Dec 15, 2017 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

rosskevin Dec 15, 2017 •

edited

Loading

oliviertassinari Dec 15, 2017 •

edited

Loading

rosskevin commented Dec 15, 2017 •

edited

Loading

rosskevin commented Dec 15, 2017 •

edited

Loading

rosskevin commented Dec 15, 2017 •

edited

Loading

rosskevin commented Dec 15, 2017 •

edited

Loading

kof commented Dec 16, 2017 •

edited

Loading

kof commented Dec 16, 2017 •

edited

Loading

kof commented Dec 16, 2017 •

edited

Loading

Bnaya commented Dec 16, 2017 •

edited

Loading

pelotom left a comment •

edited

Loading

oliviertassinari commented Jan 9, 2018 •

edited

Loading

oliviertassinari commented Jan 9, 2018 •

edited

Loading

oliviertassinari commented Jan 10, 2018 •

edited

Loading