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.

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

Jump to bottom

[Core] remove style-propable mixin from components #2852

Closed
90 tasks done
yongxu opened this issue Jan 9, 2016 · 30 comments
Closed
90 tasks done

[Core] remove style-propable mixin from components #2852

yongxu opened this issue Jan 9, 2016 · 30 comments
Labels
umbrella For grouping multiple issues to provide a holistic view
Milestone
0.15.0-alpha.1 Re...

Comments

@yongxu
Copy link
Contributor

yongxu commented Jan 9, 2016

style-propable is a mixin that I really don't see why it should exist. This mixin does not affect component's life cycle and the only thing it does is inject helper functions.

This mixin appears in most of the components and is the only mixin in most cases. Removing this not only improves the performance and clarity, more importantly, many components will be changed to pure and written in es6 style.

List of components:
@newoga
Copy link
Contributor

newoga commented Jan 9, 2016

@yongxu The long term plan is to remove all the mixins, and work on removing style-propable has already started for some of the components. For example, style-propable has been replaced with style utils in DropdownMenu, Divider, and internal stateless TextField components. That being said, this could be an issue to track components that are left. Do you want to compile a list of component that still use the style-propable mixin so we can track it?

@alitaheri alitaheri added this to the 0.15.0 Release milestone Jan 9, 2016
@alitaheri
Copy link
Member

@newoga A simple search for style-propable would reveal all the components using it. 😁

@yongxu Thanks for raising this. We'll keep track of it so we don't forget 👍 👍

@newoga
Copy link
Contributor

newoga commented Jan 9, 2016

@alitaheri I know 😄 Just figured it'd be nice to track in this Github issue as well, as since @yongxu opened this, might be nice to see at the top of the issue.

@alitaheri
Copy link
Member

Ohhh you mean a checklist we can track the progress of. I like that idea, yeah that sounds very good 👍 👍 👍

@yongxu
Copy link
Contributor Author

yongxu commented Jan 9, 2016

yeah sure thing @newoga

here are the components that currently using style-propable

Edit: Place at the top

@yongxu
Copy link
Contributor Author

yongxu commented Jan 9, 2016

there are so many of them, but it is actually very easy to track, just search ./mixins/style-propable you will be able to find all of them

@alitaheri alitaheri added Core umbrella For grouping multiple issues to provide a holistic view labels Jan 9, 2016
@alitaheri alitaheri changed the title [better implementation] mixins/style-propable.js should be removed [Better Implementation] remove style-propable from components Jan 9, 2016
@yongxu
Copy link
Contributor Author

yongxu commented Jan 9, 2016

Thanks for the change @alitaheri ! Really like the check list :)

@alitaheri
Copy link
Member

Thank you. Now we can track the progress 😁 😁

@newoga
Copy link
Contributor

newoga commented Jan 9, 2016

@yongxu @alitaheri Perfect, thanks much! (Those are a lot 😨). I'll make sure to mark my progress here as I remove uses of the mixin. Based on my memory, this mixin was going to be one of the easier ones to remove.

@newoga newoga changed the title [Better Implementation] remove style-propable from components [Core] remove style-propable mixin from components Jan 9, 2016
@yongxu
Copy link
Contributor Author

yongxu commented Jan 10, 2016

Today I was thinking about the best ways to replace style-propable, It looks like that styles.js and immutability-helper.js are the main cause that significantly slow down the performance. I am wondering if it is better to remove them completely.

Plus, AutoPrefix is very costly. IMO, We should manually adding default style prefix and leave the user to decide if they'd like to use AutoPrefixer themselves.

Currently, in style-propable,

mergeStyles() {
    return ImmutabilityHelper.merge.apply(this, arguments);
 }

that in immutability-helper,

import update from 'react-addons-update';
function mergeSingle(objA, objB) {
  if (!objA) return objB;
  if (!objB) return objA;
  return update(objA, {$merge: objB});
}

//here is the merge called
  merge() {
    const args = Array.prototype.slice.call(arguments, 0);
    let base = args[0];

    for (let i = 1; i < args.length; i++) {
      if (args[i]) {
        base = mergeSingle(base, args[i]);
      }
    }
    return base;
  },

In most cases, this is totally waste of time.
The following code


  mixins: [
    StylePropable,
  ],

//and somewhere in code, probably many times:
this.merge(defaultStyle, styleFromProps);

is simply equivalent to

{...defaultStyle, ...styleFromProps}

The two other functions mergeAndPrefix() and prepareStyles(muiTheme, ...styles) in style-propable has even worse performance.

mergeAndPrefix() ==> cost of merge() + AutoPrefix.all(mergedStyles)

prepareStyles(muiTheme, ...styles) ==> O(merge + ensureDirection + AutoPrefix.all)

Those two functions are highly used in React render function. I think this might slow down the response time x10~ times. No testing, just my estimate.

There is a quite shocking result I found:
when searching ensureDirection(muiTheme, style), 175 matches across 79 Components for "prepareStyles". Many appears in render function, including most used component such as TextField, Button, Card, Textarea, List... Pretty much all the components. ensureDirection(muiTheme, style) a very costly function, which only used to ensures that style supports both ltr and rtl directions by checking every style element that might be executed hundreds of time every time some part of your page rerender.

In practice, it is not complicated to manage AutoPrefix or left-to-right direction. We should remove these from the library. @alitaheri @newoga @oliviertassinari

@newoga
Copy link
Contributor

newoga commented Jan 11, 2016

@yongxu I agree that ideally these should be removed entirely. Right now there are discussions about using another library/tool/approach for styling and I think any significant work in this area will probably have to wait until we make that decision. I haven't fully considered the short term implications of not using those methods in existing components, but I can't imagine it not being a breaking change. I think we should carefully consider all alternatives before making this switch and try to get it right the first time.

In practice, it is not complicated to manage AutoPrefix or left-to-right direction. We should remove these from the library.

I would like to give users more flexibility in this process, but do you have an ideas how this could work?

@alitaheri
Copy link
Member

@yongxu I couldn't agree more even if I wanted to! We have a plan for these. first and most importantly we should remove a huge number of lines of code in 0.15.0 so we can move more freely, right now too many things are deprecated. and having them around really ties our hands behind our backs. With those codes removed we can do a whole lot of things. one of them would be memoization of calculated styles, optimization of these functions, using better libraries like Radium and Recompose and a whole lof of things. but with all these old rusty codes around it's really hard.

@yongxu
Copy link
Contributor Author

yongxu commented Jan 11, 2016

@newoga IMO, manually adding prefix is more efficient. The idea is actually very simple:

transition: ...

can be rewritten as:

transition: ...
WebkitTransition: ...
msTransition: ...

So we can simply write

this.prepareStyles(styles1, styles2);
/*and*/
this.mergeAndPrefix(styles1, styles2);

as

{...styles1, ...styles2}

@alitaheri Yeah I totally agree with you. IMO, If we are going to remove style-propable, the mixin that deals with style merge, we should write the whole mechanism to make it as efficient as possible. If we simply replace them with the current helper function there won't be much performance improvement and we probably have to rewrite this part again in the future. It is probably the best time to redesign the style merge and resolve this issue once for all.

Radium is a good library, but also slow. For many basic components such as buttons or text fields, manually adding prefix is the most performance efficient. It is worth to put a little extra time since one site may contain hundreds of these elementary blocks.

@alitaheri
Copy link
Member

Radium is a good library, but also slow. For many basic components such as buttons or text fields, manually adding prefix is the most performance efficient. It is worth to put a little extra time since one site may contain hundreds of these elementary blocks.

I think the slowness is negligible if it happens only once. I mean with memoization we shouldn't worry so much about prefixing. But with manual prefixing there will be many things to worry about, things like maintenance, more things to teach contributes, much harder code review, etc, etc... . I'm always against doing things that external libraries can do better much better myself. @oliviertassinari @newoga Thoughts?

@yongxu
Copy link
Contributor Author

yongxu commented Jan 11, 2016

@alitaheri Manual prefixing everything is indeed quite a lot work and much harder code review. I totally agree with you on this. However, memoization may cause internal state. I think the 20-80 rule is probably a good choice here, focus on prefixing the low level components such as Paper, and keeping code clarity for some not so frequently used components like DatePicker.

Paper is probably the most used component, but if you take a look at its render function, there is definitely quite a bit problem here:

  render() {
    const {
      children,
      circle,
      rounded,
      style,
      transitionEnabled,
      zDepth,
      ...other,
    } = this.props;

    const styles = {
      backgroundColor: this.state.muiTheme.paper.backgroundColor,
      transition: transitionEnabled && Transitions.easeOut(),
      boxSizing: 'border-box',
      fontFamily: this.state.muiTheme.rawTheme.fontFamily,
      WebkitTapHighlightColor: 'rgba(0,0,0,0)',
      boxShadow: this._getZDepthShadows(zDepth),
      borderRadius: circle ? '50%' : rounded ? '2px' : '0px',
    };

    return (
      <div {...other} style={this.prepareStyles(styles, style)}>
        {children}
      </div>
    );
  },

Transitions.easeOut() is quite bad, it is simply generating '450ms all 0ms linear' over and over again.
this.prepareStyles(styles, style) is terrible, iterating all the styles, slow merging and checking if everything supports both ltr and rtl and then prefixing every element... umm this is definitely a problem.

@alitaheri
Copy link
Member

Manual prefixing everything is indeed quite a lot work and much harder code review

There is also server-side-rendering. without user-agent aware prefixing we'll end up with tons of useless prefixed going down the wire. surely a very new browser will never need any prefix. we'll have to add a huge number of bytes only to supper a very few number of target browsers. Such things can be very easily handled by libraries. Surely they are slow, but we don't need that much performance if we handle the flow right. (with memoization)

memoization may cause internal state.

Why do you say that? I can't imagine how memoization can do this if we handle things right. Also handling memoization right is A LOT easier than handling prefixing right. I for one know exactly how to memoize with ease and determinism. But I truly suck at prefixing 😢 😢

Just a side note. I mean absolutely no offense. you points are valid. And I really enjoy brainstorming 😁 Thank you for taking the time to discuss this in the first place, 👍 👍

@oliviertassinari
Copy link
Member

Regarding your first point, I completely agree, we should move our logic out of those mixins 👍.

this.merge(defaultStyle, styleFromProps);

is simply equivalent to

{...defaultStyle, ...styleFromProps}

What's the point?

The two other functions mergeAndPrefix() and prepareStyles(muiTheme, ...styles) in style-propable has even worse performance.

mergeAndPrefix() is deprecated, we are supposed to only use prepareStyles() in the entire code base.

`prepareStyles(muiTheme, ...styles) ==> O(merge + ensureDirection + AutoPrefix.all)``

The ensureDirection work is bypassed unless you enable the RTL mode.

ensureDirection(muiTheme, style) a very costly function

I'm not sure about it, but we could use a hash map instead of a switch to improve performances for people how use the RTL option.

Those two functions are highly used in React render function. I think this might slow down the response time x10~ times. No testing, just my estimate.

Yes, prepareStyles is highly used in our component.
Let's take a look at the DatePicker component that is probably the one how use it the more (each date displayed in the calender is a react component: DayButton).
Here is the result. In order to produce it, I have just recorded what happens when we open the DatePicker dialog.
capture d ecran 2016-01-11 a 19 53 19

The prefix job seems pretty cheap.

In practice, it is not complicated to manage AutoPrefix or left-to-right direction. We should remove these from the library.

How would you do it?

@oliviertassinari
Copy link
Member

That's also related to #2437.

@newoga
Copy link
Contributor

newoga commented Jan 11, 2016

@yongxu @alitaheri @oliviertassinari I have a suggestion that basically sums what was presented in this issue and proposes a next step forward. 😄

1. Replace style-propable mixin with utility functions

I think we would all agree that these functions do not need to be in a mixin. We have an exported utility library at /utils/style.js that pretty much uses the same implementation as the mixin. Removing the use of the mixin will be a step towards transitioning the codebase towards functional components and class based components (and cleaning up the code in general).

The style-propable isn't the only mixin we have to remove, but it probably is the most straight forward one to remove. I think this issue would be a great umbrella issue to track the progress of that initiative.

2. Finalize how we should change and use /utils/styles.js

The /utils/styles.js module currently exports 4 functions:, merge, mergeAndPrefix, ensureDirection, and prepareStyles.

We're not using mergeAndPrefix since its deprecated, so I say remove all uses of it (if any) and simply remove this function from the /utils/styles.js module.

The prepareStyles function can't be removed for backwards compatibility reasons.

I'm not sure if there is any reason why ensureDirection needs to be exported if it's only for prepareStyles, so maybe we can leave this function, but not export it.

That really only only leaves merge. @alitaheri @oliviertassinari, do you think we should continue using this function or should we switch all uses of this to the destructuring syntax?

3. Move discussions of how to change styling to a different issue

@yongxu I think everyone agrees that the current approach to styling is going to have to change. The problem is that there is still a ton of work to be done in a number of areas before we can tackle it.

Would it be alright if we continued or postponed this discussion of manual AutoPrefixing and direction (RTL) into a separate issue?

@alitaheri
Copy link
Member

Great sum up @newoga 👍 👍 I agree on all fronts 😁 Thank you

@yongxu
Copy link
Contributor Author

yongxu commented Jan 11, 2016

@alitaheri

memoization may cause internal state.
Yeah you are right, memoization does not necessarily introducing internal state, depends on how the code is handled. Depends on the situation, you either need extra initialization or using react life cycle method. IMO, it is probably not the ideal choice for UI or stateless components.

I'd love to see more ideas and different opinions. No worries your point is not offencive at all. I'd like to see all different opinions even if they are offencive, as long as they are valid. 👍

@oliviertassinari this.merge(defaultStyle, styleFromProps); is not very costly, but it does run quite a bit unnecessary code. {...defaultStyle, ...styleFromProps} is much more straightforward and clear.

The ensureDirection work is by passed unless you enable the RTL mode.

Your are right on this, sorry I did not see this line if (!muiTheme.isRtl) return style;. However I still think we should remove this extra logic, let user to deal with it.

I also tried to run few components using chrome timeline. The cost were probably not as high as I expected initially, they are still quite costly and should be optimized. Not very familiar with chrome timeline, but I think the point is still valid.

@oliviertassinari
Copy link
Member

Babel transpile {...defaultStyle, ...styleFromProps} using this utility method:

var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

It would be intereseting to see the performance differences between _extends() and merge().

let user to deal with it

The thing is, I can't see how users can deal with RTL on their side.

This was referenced Jan 11, 2016
Merged
Merged
@yongxu
Copy link
Contributor Author

yongxu commented Jan 13, 2016

@oliviertassinari merge uses react-addons-update, performance wise it is surely slower and makes code more complicated. There is no reason to import an addon if it is not necessary.

RTL can be dealt by rewriting style, maybe we can put this part of code into theme manager. Checking each style element in every rendering is just too much overhead and not very reasonable.

oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3219 from newoga/#2852/menus
854d18d 
[Menus] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3235 from newoga/#2852/avatar
61014dc 
[Avatar] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3229 from newoga/#2852/left-nav
9283a6f 
[LeftNav] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3236 from newoga/#2852/tabs
b473a35 
[Tabs] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3233 from newoga/#2852/progress
5a08878 
[Progress] Remove style propable mixin
oliviertassinari added a commit that referenced this issue Feb 8, 2016
@oliviertassinari
Merge pull request #3198 from newoga/#2852/card
d88e943 
[Card] Remove style-propable mixin
alitaheri added a commit that referenced this issue Feb 8, 2016
@alitaheri
Merge pull request #3227 from newoga/#2852/table
eb4435c 
[Table] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3251 from newoga/#2852/floating-action-button
dcea1da 
[FAB] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3257 from newoga/#2852/snackbar
c6f1d45 
[Snackbar] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3259 from newoga/#2852/list
ad52f02 
[List] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3254 from newoga/#2852/enhanced-button
88d1af9 
[EnhancedButton] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3260 from newoga/#2852/SelectField
b79b2a8 
[SelectField] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 10, 2016
@oliviertassinari
Merge pull request #3252 from newoga/#2852/enhanced-textarea
c10f63d 
[EnhancedTextArea] Remove style-propable mixin
alitaheri added a commit that referenced this issue Feb 11, 2016
@alitaheri
Merge pull request #3256 from newoga/#2852/ink-bar
b8dd250 
[InkBar] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 11, 2016
@oliviertassinari
Merge pull request #3263 from newoga/#2852/raised-button
0f1b16f 
[RaisedButton] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 11, 2016
@oliviertassinari
Merge pull request #3261 from newoga/#2852/selectable-enhance
cee2801 
[SelectableEnhance] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 11, 2016
@oliviertassinari
Merge pull request #3291 from newoga/#2852/date-picker
d7570ef 
[DatePicker] Remove style-propable mixin
oliviertassinari added a commit that referenced this issue Feb 11, 2016
@oliviertassinari
Merge pull request #3255 from newoga/#2852/enhanced-switch
722f9d7 
[EnhancedSwitch] Remove style-propable mixin
@rob0rt rob0rt mentioned this issue Feb 12, 2016
Closed
7 tasks
@newoga newoga mentioned this issue Feb 13, 2016
Merged
oliviertassinari added a commit that referenced this issue Feb 15, 2016
@oliviertassinari
Merge pull request #3349 from newoga/#2852/TextField
523c3a4 
[TextField] Remove style-propable mixin
@oliviertassinari
Copy link
Member

That's pretty awesome 🚀. @newoga Good job!

oliviertassinari added a commit that referenced this issue Feb 17, 2016
@oliviertassinari
Merge pull request #3351 from newoga/#2852/deprecate-style-propable
09b4c68 
[Core] Deprecate style-propable mixin
@bytor99999
Copy link

Sorry to comment on a closed ticket, but it seems previous devs on our app used the internal material-ui versions of style-propable mixin in our own code, and now that material-ui has removed it, our code now breaks and I need to figure out how to remove the coupling in our code. Any suggestions?

@newoga
Copy link
Contributor

newoga commented Mar 11, 2017 •
edited
Loading

@bytor99999 I'm sorry to hear that. There are a handful approaches to tackling a migration like this and it probably all depends on how large your codebase is, how coupled you are to the mixin (including which methods you are using), and how much manual effort you are willing to invest.

The old mixin implementation is here: https://github.com/callemall/material-ui/blob/v0.14.4/src/mixins/style-propable.js

You could probably reimplement the mixin in your local project and change all the import paths from material-ui to your new local module. That would probably be the least impact on your current code, however that being said, your larger goal should probably be to move away from mixins entirely as the React community is moving away from it.

To see how this project moved away from the mixin, you can look at any of our closed PRs that show how we did it for each component: https://github.com/callemall/material-ui/pulls?q=is%3Apr+style-propable+is%3Aclosed. Rather than blindly following the same approach we took, you may want to consider changing your code to not use material-ui at all for style-propable related functionality. If my memory is correct, the mixin before was pretty much for auto-prefixing and merging objects, all which can be done by importing and using other community packages yourself.

If you have a really large codebase, my feedback would be to:

  1. Slowly migrate your code away from the mixins before you update to a version of material-ui that didn't include it.
  2. Optionally consider scripting parts of the migration using a tool like https://github.com/facebook/jscodeshift

Good luck!

Edit: The original comment to this issue has a links to each PR where the mixin was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Milestone
0.15.0-alpha.1 Release
Development

No branches or pull requests
6 participants
@chrismcv @newoga @bytor99999 @yongxu @oliviertassinari @alitaheri