[Discussion] Utilize react-memo

[alitaheri]

react-memo uses memoization to enhance performance of react components.

This is a demonstration of it's api. Although it still has a lot of work ( I researched for 4 days and made it in 3 hours, 80 LOC ). heavily inspired by Recompose and Reselect. it aims to somehow efficiently transform the props that are passed down.

I have migrated Divider since it's small only to demonstrate this library.

I'll provide a tiny walk through on the code.

also note that using this with pure: true will only re render if any of the selectors return a new value for the new props. it's much more efficient that shallow comparing props, but very prone to errors, it must be used with caution.

@oliviertassinari @newoga @mbrookes Take a look please.

[alitaheri]

they can also work with context: (props, context) => ...

[oliviertassinari]

@alitaheri That's a very interesting solution 😍.
When we use the pure function approach, the styles are computed later down the pipeline. That allow us to run the shallowEqual() on less properties 👍.

I just have one concern so far. We would have to move the state out of our components to fully enjoy this solution or at least break our components into stateless one. That makes me remember that we would have to do the same if we use Radium.

[alitaheri]

We would have to move the state out of our component to fully enjoy this solution.

Not really. some of these can break down. after all a setState will only trigger a render within the subtree. so if some of these styles are overriden inside the render function according to state it won't matter much. this will reduce allocations dramatically already. I was worried about state myself. but then I realized we plan on removing state. and even if it should have state, it won't be that bad 😁

[alitaheri]

When we use the pure function approach, the styles are computed later down the pipeline. That allow us to run the shallowEqual() on less properties

At first I wanted to also run shallow equal on props and context too. but then I realized Recompose's pure can do that! my pure is going to stay very dangerous and mostly discouraged. but in case some components are safe to use it. I thought it might not be bad to place it. you can take a look at the the code and see why pure is dangerous ( it doesn't react to other props, only selector results) but it's a nice feature. Recompose has it too 😁

[chrismcv]

Just having a nosey through the comments here. Feel free to ignore me as I haven't been part of the chat much so far...

I feel like it is a lot of work here, and is giving us performance, but possibly not a great deal beyond that. After going through SelectField perf issues last week, and concluding that inline-styles pose a significant challenge, I've been thinking a lot about different solutions, and am probably landing on a preference for something className based, rather than inline-style based.

If https://github.com/martinandert/react-inline had support for webpack/browserify, I would be strongly advocating for it as the ideal solution. It allows js based styles alongside css based power (e.g. media queries), performance and "overide-ability", which I think is a big win for users of the library. .mui-AutoComplete .mui-ListItem {font-size:14px} (or less so <AutoComplete className={styles.myCustom} /> is much more convenient and familiar for users, and also significantly reduces the surface area of components, particularly those that are composed more often. A className approach also makes shallowEquals automatically work, rather than the effort required with memoization.

[newoga]

I'll just put my 2 cents here but @oliviertassinari @alitaheri @mbrookes feel free to share your thoughts (in agreement or disagreement) as well.

Just having a nosey through the comments here. Feel free to ignore me as I haven't been part of the chat much so far...

First of all, it's not nosey since this discussion is happening in a public forum! To you and everyone else that is reading, I strongly believe that the success of this project is dependent on the contributions from everyone and while I'm working on material-ui, I plan to do my part to keep it as open of a community as possible. 😄

Now for my opinion:

Even though there might be some overlapping goals and benefits, I don't think the intention behind these changes were so that we didn't have to implement an alternative styling library such as react-inline or any of the other number of options discussed in other issues (#684, #1951). That being said, at the moment, memoization is probably the most straight forward approach to improving performance and eliminating wasted render time just because we're able to wrap existing code rather than rewrite code.

In my opinion, the practical challenge with migrating to react-inline is that we would have to carefully go through each of the material-ui components and separate static styles into the Stylesheet.create function while retaining dynamic styles inline (and material-ui has a lot of dynamic styles). We'd also have to do that while being sensitive to the exposed styling prop APIs to either maintain backwards compatibility or define a clear migration path. It would be a huge effort. I'm not saying that it isn't worth the effort, but due to the amount of work involved, my personal preference would be to make the decision after:

1. We're more well informed about all the existing style framework alternatives.
2. Current API surface area has been sufficiently reduced and simplified
3. Code quality has been improved to the point we're comfortable refactoring styles without introducing regressions

For the record, I love the idea of extracting inline styles (or more specifically, I like to call them javascript defined styles) to stylesheets and I think there's still a lot innovation to be seen in this area. 😄 But in many ways, I think the issue with implementing it is a human problem, not a technical one.

[mbrookes]

What @newoga said.

Far more eloquently put than I could have (especially at this time of night! 😴 )

[alitaheri]

Yeah @newoga kinda said everything I had in mind 😅 😅

[alitaheri]

@oliviertassinari @newoga @mbrookes I finalized react-memo. I added some tests + documentation. If you guys are ok with how it works, I'll continue on this PR 😁

[newoga]

@alitaheri Congrats and awesome work! Thanks for the docs too. It looks very cool! 😄

Question: I know we plan on moving towards stateless components, but for components that currently have state, would we need react-memo selectors also handle state for memoization, such as (props, context, state) or if this is the case (props, state, context) may be more intuitive?

Let me know, you've surely thought a lot of this through!

Unrelated:

Something I want to discuss with you guys is I think we should create a strategy/pattern of exporting our components. We should export plain versions of our components that aren't wrapped for shallow testing purposes. I think there could be reasons other than shallow testing why that makes sense, for example maybe it could be helpful for code reusability for react-native.

We could investigate after we reorganize the directories, but we should probably decide on this before we migrate all the components to muiThemeable.

[alitaheri]

@newoga

Something I want to discuss with you guys is I think we should create a strategy/pattern of exporting our components.

Absolutely!! I was thinking about this earlier. I believe if we have one folder per component things will be a lot easier! we can export many versions of our components (stateless, stateful, wrapped, plain, native, etc... )

I know we plan on moving towards stateless components.

There is nothing wrong with recalculating some parts of the style inside the component itself. We can't really move the state in the memo since it can't really track changes of it's child, or to make matters worse it can't know how deep the actual component is if we have some other HOCs there. I believe it's not all that bad to have some of the calculations done inside the component. In my opinion we can extract as much calculations as we can and leave some for the state. after all, it will be one extra call to Object.assign 😁

[alitaheri]

Introducing memoizeStyles

So I came up with a way to change how the memos would be written. The memoizeStyles utility helps writing styles in this way:

import memoizeStyles from './utils/memoizeStyles';

const styles = {
  stuffStyle: {
    // static styles
  },
  otherStuffStyles: [
    (props) => props.foo,
    (props) => props.bar,
    (foo, bar) => ({baz: foo + bar}), // memoized styles
  ],
  scaryStuffStyle: () => ({
    // more static stuff, but instead of a global object
    // it will be one object per instance. this is preferable
    // since it can avoid some subtle bugs.
  }),
  // ...
};

// and then:

const Component = memoizeStyles(styles)(Component);

Prettier, ey? 😁

I also Migrated Subheader just for the argument's sake.

@newoga @oliviertassinari @mbrookes Please take another look at this and tell me what you think.

[oliviertassinari]

I like this memoizeStyle!
That makes me think at how React Look is handling the dynamic style. Have a look at the example of https://github.com/rofrischmann/react-look/blob/develop/README.md.

  title: {
    fontWeight: 800,
    // use functions to inject props, state or context values
    fontSize: (props, state, context) => props.size * state.zoom
  }

Remember when I was wondering how I was going to use the context to dynamically style the native component? Well, React Look sounds perfect for this use case. I'm wondering if the style is memoized.

[robinweser]

@oliviertassinari I am right now writing some technical docs on how Look works under the hood, but it will take some time :P Basically I can say that there's StyleSheet.create which separates static and dynamic styles. Both are added to the StyleContainer which collects all styles and renders those static styles to pure CSS classes.
Dynamic styles are referenced by the className they're used in and get resolved every time a Component re-renders. After resolving them a content-based hash is generated (used as className). This makes even dynamic styles reusable as you do not need to add the same dynamics over and over again.
Feel free to ask or chat if there are any questions/ideas/improvements. Hard to describe everything in short right now, but make sure to check the "under the hood" docs soon.

[nathanmarks]

@alitaheri

Where do we sit right now in terms of implementing style memoization or other style-related improvements? This is an area I keep running into when debugging performance related concerns brought up in issues.

Is this a post-0.15 concern?

[alitaheri]

@nathanmarks There are a couple of other concern that arise bu using this. I am working ( been working on it for a long time, I'm busy these days 😞 😞 ) We must handle all those concerns at once, we can't ease into it. using HOC breaks imerative methods and sometimes refs ( no a big deal ), also this doesn't handle state very well, so components should become as stateless as possible to fully utilize the power of memoization, also if we don't we'll have styling logic spread around a LOT. prepareStyle() cannot be called more than once! and it's heavy calculation, in other words, it should be inside the resolver not in render(). I can go on and on. I have to figure out other ways to handle some these concerns, specially state! before continuing this.

[nathanmarks]

Thanks for the reply @alitaheri , totally understand the difficulty in the challenge presented here with all the different parts.

If I have any ideas for some of the library-wide style/performance issues I will post them here for you to check out.

[alitaheri]

@nathanmarks Thank you, that would be truly appreciated 👍

And for react-look, @nathanmarks @oliviertassinari It does seem promising, Should we investigate that further?

[alitaheri]

But maybe it's debatable, with what you proposed we can also move memoization logic there. Like how I do it with react-memo.

[chrismcv]

@alitaheri - thinking out loud... maybe themes should be a stylesheet object too? Then we can just use combineStyles?

e.g.

const muiTheme = Stylesheet.create({
  paper: {
    backgroundColor ...
  }, 
  ... etc
})

Then in the component styles={(c(styles.root, this.state.muiTheme.paper, .....)}

[chrismcv]

fyi, here is a working version... chrismcv@106705b

I think this is the shortest we can make the paper definition at the moment.

const styles = StyleSheet.create({
  root: {
    backgroundColor: (props, {muiTheme}) => muiTheme.paper.backgroundColor,
    borderRadius: ({circle, rounded}) => circle ? '50%' : rounded ? '2px' : '0px',
    boxShadow: ({zDepth}, {muiTheme}) => muiTheme.paper.zDepthShadows[zDepth - 1], // No shadow for 0 depth papers
    boxSizing: 'border-box',
    color: (props, {muiTheme}) => muiTheme.paper.color,
    fontFamily: (props, {muiTheme}) => muiTheme.baseTheme.fontFamily,
    transition: ({transitionEnabled}) => transitionEnabled && Transitions.easeOut(),
    WebkitTapHighlightColor: 'rgba(0,0,0,0)', // Remove mobile color flashing (deprecated)
  },
});

[alitaheri]

maybe themes should be a stylesheet object too? Then we can just use combineStyles?

Really interesting... We should investigate that approach too 👍

[robinweser]

@chrismcv I see. Seems like a legit use case to have a whole selector as a function of (props, state, context). For now there's no better option, but I will try to add this feature today. 👍

[chrismcv]

Slightly off topic here... but I see @pradel is adding lots of tests, and testing the styles that are applied, which is awesome. But... in the context of a potential move to react-look and it's CSS based approached, we should possibly consider building the style checks with getComputedStyle (https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle). This way we 1) wouldn't have to fix tests that are broken 2) the tests will confirm that we're not introducing regressions.

[alitaheri]

I agree 👍 That's interesting 👍

[nathanmarks]

@alitaheri @chrismcv

I am on the same page in terms of holding off on too much style testing until we figure out the style situation.

However, the unit tests do not have a real DOM or browser APIs. Testing using getComputedStyle belongs in the browser testing section, which runs using karma.

The style assertions in the unit tests, as long as we are doing inline styling via JS with the current method, help confirm that the component is working as intended as a unit. (So long as you don't end up testing prepareStyles 😄).

As you mentioned though @chrismcv -- this is not going to mesh with different styling implementations such as react-look. So with an implementation change looking likely, I think that you're right in suggesting using getComputedStyle() so that tests do not have to be refactored.

@alitaheri Should we state moving forward that style assertions belong in the browser tests?

[nathanmarks]

Also, remember that in the unit tests, checking props('style') or wrapper.node.style.xxx is not checking the rendered output, rather it is checking if the react node (root or otherwise in the render function) was passed the given styles. This is definitely useful for parents that convert top level props into styles for their inner elements/components.

Or would react-look change that too?

[alitaheri]

@nathanmarks Yes it will, react-look, basically converts inline styles to css, hashes them, use the hash as key with prefix and put the style in the head. Then you can get the hash and assign it to a component. So it does change that, and the only way to test it is using getComputedStyle()

[nathanmarks]

@chrismcv Here's an example to go with my last reply:

Component:

const Component = ({color}) => (
  <div style={{backgroundColor: color}}>Hello</div>
);

Test:

const wrapper = shallow(<Component color="red" />);
assert.strictEqual(wrapper.node.props.style.backgroundColor, 'red');

Unless I'm mistaken, node.props.style.backgroundColor is still an assertion we'd want to make at the unit level.

Imagine <div> is a component with react-look. This is not rendering the <div> component and checking the output, it's just asserting that we took the color prop and passed it as the style.backgroundColor to the <div />.

The getComputedStyle() output seems suited for integration/browser tests and to properly assert final, rendered styles.

[nathanmarks]

@alitaheri if we are testing the react-look integration (with the full DOM and browser API needed), it isn't really a unit test imo.

I totally agree that we need to test that properly -- but let's try keep the unit tests separate from these concerns.

[alitaheri]

@nathanmarks I think it is. I mean react-look isn't a different part on this library. we assume it works as expected ( it has a good coverage ). Relying on that fact, we can do unit tests and make sure the components behave as expected by inspecting their computed styles.

P.S. I'm not an expert on testing, correct me if I'm wrong please 😅

[nathanmarks]

@alitaheri It's still testing the integration with react-look. In the unit tests we never fire componentDidMount as we do not actually mount the components on a DOM.

Same reason I want to be able to export these easily without HoCs -- so we can unit test the code at the core of the component.

Anything that depends on being mounted on a real DOM (such as writing a style element) belongs in the test/browser section, not the test/unit section.

There is a full virtual DOM we could use to simulate this, but I think it's better just to do that in a real browser since we have that setup. Using jsdom in the unit test suite might result in overcomplicated "unit" tests.

[chrismcv]

@nathanmarks ok - testing may prove more challenging with react look.

e.g.
https://github.com/callemall/material-ui/pull/3528/files#diff-a10f389f4f83f677dc2aa8259bf1f748R88

it('renders children and change zDepth', () => {
+    const zDepth = 3;
+    const wrapper = shallow(
+      <Paper zDepth={zDepth}>{testChildren}</Paper>
+    );
+
+    assert.ok(wrapper.contains(testChildren), 'should contain the children');
+    assert.equal(wrapper.prop('style').boxShadow, theme.paper.zDepthShadows[zDepth - 1],
+      'should have good zDepthShadows');
+  });
+});

The react-look way to do this is

const styles = StyleSheet.create({
  main: {
    boxSizing: 'border-box',
  },
  stateBased: (props, state) => {
    const {
      circle,
      rounded,
      transitionEnabled,
      zDepth,
    } = props;

    const {
      baseTheme,
      paper,
    } = state.muiTheme;

    return {
      color: paper.color,
      backgroundColor: paper.backgroundColor,
      transition: transitionEnabled && Transitions.easeOut(),
      fontFamily: baseTheme.fontFamily,
      WebkitTapHighlightColor: 'rgba(0,0,0,0)', // Remove mobile color flashing (deprecated)
      boxShadow: paper.zDepthShadows[zDepth - 1], // No shadow for 0 depth papers
      borderRadius: circle ? '50%' : rounded ? '2px' : '0px',
    };
  },
  override: (props) => props.style,
});

Because the border-radius is now getting added to a style-sheet, we lose the ability to test this. Unless we view the stylesheet as a unit and the component as a separate one?

[nathanmarks]

@Chrismvc the browser tests run with karma can take care of the react look output much more easily. We have both browser and unit tests.

[alitaheri]

Hmmm You are right. we should add some testing capabilities within the react-look library! Like extracting the computed, inherited final style instead of just getting the className. how will that work?

[robinweser]

Seems like we're heading in the same direction.
Actually 30min~ ago I thought about that too ( https://github.com/rofrischmann/react-look/issues/226 ). I will start reorganising the project structure in less than an hour. Then I'll start to clean the test files and create some test utils. We might export those for other libs as well. e.g. some helpers to actually get the extracted styles for a Component, etc.

[nathanmarks]

I'll have to take a look at react look, either way I'll make sure we set things up in a way that they are testable.

Do you see the difference between what we are testing in the unit vs browser tests?

The idea with the unit tests is to assert behaviour before react look has stuck it's fingers in. This includes styles passed as props. (If there are any)

[newoga]

We might want to move this discussion to a new issue (we can focus on memoization here). Even though a change to the styling approach is likely, it's not going to happen in 0.15.0. There are enough moving parts already for this upcoming release. That being said I don't think we're necessarily wasting any effort by testing using our current approaches.

[nathanmarks]

@alitaheri I'm closing this for now, feel free to re-open if we use this PR again.

[alitaheri]

I forgot to close it myself. thanks 👍