[Core] Initial style refactor

[newoga]

@oliviertassinari @alitaheri

This is the start of some of the refactors that I wanted to do, but there is still more. So far I've done the following:

Simplifies style utilities and removes utils/immutability-helper.js

This commit includes a number of changes:

1. Removes all apply() calls in favor of argument destructuring
2. Simplifies style-propable mixin to import/wrap solely from utils/styles
3. Search the code base for components that used utils/immutability-helper and replaced it with utils/styles/mergeStyles or react-addons-update
4. Removed utils/immutability-helper from src
5. Renamed utils/styles merge() to mergeStyles()
6. Prepared deprecation of passing more than one style to prepareStyles() (it is imlemented but commented)
7. Fixed all components that are dependent on utils/styles to follow new convention (do not import utils/styles default export to a styleUtils object, just import the method you need)

Todo:

1. Refactor/cleanup ensureDirection
2. Decide on whether to use mergeStyles or use spread operators in components
3. Investigate opportunities for memoization

[newoga]

@oliviertassinari @alitaheri

What are your thoughts on removing immutability-helper completely? (yay for less API surface area!) If we do that, I would like to move its merge implementation into styleUtils that way they aren't exported in two places.

With this PR, the only place immutability-helper is being used is in touch-ripple.jsx for its push() and shift() functions. I'm not sure how important it is to wrap react-addons-update if we use it so infrequently. The theme-manager.js and menu.jsx currently use react-addons.update directly, so I think it's probably okay for touch-ripple.jsx to use it directly too, or just use destructuring syntax. Let me know your thoughts...

And of course, we can also deprecate immutability-helper and remove all uses of it internally until 0.15.0 as well.

[newoga]

Also, let me know your thoughts on renaming prepareStyles to prepare in the styleUtils for consistency.

That way we have:

/* Old mixin behavior */
this.mergeStyles()
this.prepareStyles()

/* util */
styleUtils.merge()
styleUtils.prepare()

I suppose a third approach is to leave the method names the same (change merge back to mergeStyles), then you could do something like this:

import {mergeStyles, prepareStyles} from '../utils/styles'
/**
 * Also works like ...
 * import * as styleUtils from '../utils/styles';
 * styleUtils.mergeStyles();
 * styleUtils.prepare()
 */

const SomeComponent = ({muiTheme, style}) => {
  const rootStyle = {};
  return (
    <div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
  );
};

To be fair, fourth approach is just to abandon merge all together and use destructuring like @yongxu mentioned.

import {prepareStyles} from '../utils/styles'

const SomeComponent = ({muiTheme, style}) => {
  const rootStyle = {};
  return (
    <div style={prepareStyles(muiTheme, {...rootStyle, ...style}}/>
  );
};

Sorry for dropping all of this, but we'll soon be ready for removing style-propable and just want to make sure we agree on a syntax that we're happy with.

[alitaheri]

Thanks a lot @newoga, This is a big help 👍

What are your thoughts on removing immutability-helper completely

I completely agree! It's not that useful and it's very confusing! I mean, there are many libraries to handle such things. why do we have our own.... oh wait it WRAPS another library :|

I think merge must be removed 😁 and prepareStyles cannot be confused with prepare. I think prepare is easier to write without confusion.

[oliviertassinari]

What are your thoughts on removing immutability-helper completely?

That sounds like a good idea. It's only used in the TouchRipple. I think that It could be replace with simple ES6 destructuration and the rest spread: https://github.com/sebmarkbage/ecmascript-rest-spread.
Well, using react-addons.update could be another way to do it.

Regarding the syntax we can use. I really like this one:

import {mergeStyles, prepareStyles} from '../utils/styles'

const SomeComponent = ({muiTheme, style}) => {
  const rootStyle = {};
  return (
    <div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
  );
};

[newoga]

Thanks for your feedback guys. I made a few additional commits that I would like your feedback before continuing.

First, immutability-helper.js is gone!

@alitaheri @oliviertassinari After experimenting a bit, I decided that this syntax is the best route.

import {mergeStyles, prepareStyles} from '../utils/styles'

const SomeComponent = ({muiTheme, style}) => {
  const rootStyle = {};
  return (
    <div style={prepareStyles(muiTheme, mergeStyles(rootStyle, style)}/>
  );
};

I put details as to why in this commit message. But ultimately I realized that this pattern better follows the functional approach we are heading in as well as offers bunch of benefits as it relates to our eslinter (such as detecting unused methods and correct method names). I noticed it immediately when I made the change. Also, styleUtils.prepare is longer than prepareStyles so I think it made the code cleaner too. I can elaborate if you have questions.

[newoga]

FYI, whether we should continue using our mergeStyles implementation or switch to spread/destructuring across our components is still up and the air. I haven't evaluated performance, but I'm open to what you guys think. If we get ride of mergeStyles, this will get us down to one function, prepareStyles!

@alitaheri

I think merge must be removed 😁

Sorry, I forgot to address this. Does this mean that you think we should switch everything to destructuring? I don't disagree, I just don't know if we need to confirm performance implications. Let me know!

[yongxu]

Thank you for your awesome work guys! This looks great 👍

[oliviertassinari]

@newoga Nice work. I'm really happy with it 👍. Could you squash down the commit?
@alitaheri Are you ok to merge this?

[alitaheri]

@newoga I didn't mean we must use destructuring. I was only saying that we should leave such trivial things to external libraries like lodash (babel's polyfill). Since this is what they are really good at. For now it's all good. it doesn't matter that much anyway. this is very good for now. thanks 👍 👍 👍

@oliviertassinari Yes I'm all good

[newoga]

@alitaheri Okay right, I agree. Thanks for clarifying. 😄 There are still a few remaining pieces I want to solve, we can revisit in next PR.

@oliviertassinari @alitaheri In case you need to review anything else before merging, I made a backup of the branch before rebasing in case you need to see the separate commits.

Thanks guys, if all is green, feel free to merge.

[alitaheri]

It's all good @newoga Thanks a lot for your hard work on this 👍 👍 👍

@oliviertassinari I'll leave the merge to you. All green here 💚