Replace Object.assign with spread syntax

[denisborovikov]

Minor update to make the code more consistent and readable.

[javivelasco]

I think @vectorsize was having trouble with the performance of using the spread operator. I think it could be nice if we can put together some benchmarks. Also probably using the Object.assign straight without a polyfill it's not the best possible idea so we can include a submodule from lodash or something.

What do you think?

[denisborovikov]

Spread operator will be transpiled by babel to Object.assign anyway. I don't know anything about performance here but not sure we can win too much with such micro optimisations.

If there is performance issue in current js implementation I'm pretty sure js engines' developers should take care of it not we are. Someday they'll fix it.

[denisborovikov]

Ha ha now I see. Before #12 you had similar code :)

[javivelasco]

Yeah! I don't know the internals of the final transform and @vectorsize told me that he has a case with a lot of classes in context and stuff and that way it worked better for him. I guess the best would be measure or something, gonna try before merging. Regarding syntax I prefer spread as you might guess 💃

[denisborovikov]

Take a look at lib/components/themr.js

  return Object.keys(theme).reduce((result, key) =>
    ({ ...result, [key]: `${style[key]} ${theme[key]}` }), style)

transpiles into

  return Object.keys(theme).reduce(function (result, key) {
    return _extends({}, result, _defineProperty({}, key, style[key] + ' ' + theme[key]));
  }, style);

where

_extends = Object.assign || polyfill function

Quick googling doesn't give any results about object assign/spread performance.

[raveclassic]

Regarding performance. Here's what I've figured out.

function themeable(style = {}, theme) {
  return Object.keys(theme).reduce((result, key) => {

    const result1 = Object.assign({}, result, {
      [key]: `${style[key]} ${theme[key]}`
    })

    //

    const result2 = {
      ...result, 
      [key]: `${style[key]} ${theme[key]}`
    };

    //

    const result3 = { //make a copy
      ...result
    };
    //manually inject key
    result3[key] = `${style[key]} ${theme[key]}`;
  }, style);
}

transpiles to

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; };

function themeable() {
  var style = arguments.length <= 0 || arguments[0] === undefined ? {} : arguments[0];
  var theme = arguments[1];

  return Object.keys(theme).reduce(function (result, key) {
    var _Object$assign, _extends2;

    var result1 = Object.assign({}, result, (_Object$assign = {}, _Object$assign[key] = style[key] + " " + theme[key], _Object$assign));

    //

    var result2 = _extends({}, result, (_extends2 = {}, _extends2[key] = style[key] + " " + theme[key], _extends2));

    //

    var result3 = _extends({}, result);
    //manually inject key
    result3[key] = style[key] + " " + theme[key];
  }, style);
}

You can see that using dynamic keys always creates a new object which is then passed to _extends or Object.assign. In terms of performance the optimal way is to manually update the key on resulting object. Moreover string concatenation should only be done if style[key] exists.

UPD: It's also better not to use default args here but to put style to the last position and to conditionally reset to {}:

/**
 * Use can use JSDoc to make a hint for your IDE/Editor to not require optional args
 * @param {{}} theme
 * @param {{}} [style={}] - optional
 */
function themeable(theme, style) {
    if (!style) style = {};
}

I understand that this is not ES6 way but I strongly believe that this code is run very often and should be optimized as much as possible.

UPD2: Of cource this is all so minor that I'm sure we can live without that :)

[vectorsize]

Hi everyone, so to clarify a little my use-case:

I was passing in a css file that via a scss import came with a lot of unnecessary classes, and on top of that, re-rendering some components on page scroll, and using themr here had a big impact. The crucial part of the optimisation I submitted lies in the fact that instead of looping though every class in the passed in style, we now only loop through the theme related ones and then inject those into the passed styles.

I didn't care much for mutations in my case, and didn't test all the edge cases, some of your proposals makes sense to me, regarding the transpiling of the spread operator, now when I hear you say it, it does make sense that babel transpiles it to object.assign, when I swapped those in my code it felt like it performed better… but then again, didn't do any serious testing.

Bottom line, as long as we don't loop over unnecessary classes it will still be an improvement, regardless of the mutation/extension strategy.

[raveclassic]

@vectorsize Right now I'm handling such cases explicitly constructing child component's theme on the fly either on module level or in parent's render method.

import css from './Parent.css'; //a heavy css with lot's of things
import classnames from 'classnames';

//module level - this helps a lot with static themes because it's not constructed everytime basing on namespace
const childTheme = {
    container: css.child,
    title: css.child__title
};

@themr(Symbol(), css)
class Parent extends React.Component {
    render() {
        return <Child theme={childTheme}/>;
        //or render level
        //here I can compose theme depending on some param
        const dynamicChildTheme = {
            title: theme.child__title,
            container: classnames(theme.child, {
                [theme.child_type1]: type === '1',
                [theme.child_type2]: type === '2'
            })
        };
        return <Child theme={dynamicChildTheme}/>;
    }
}

Some words about this: I use css-modules with composition so basicly I don't need both theme.child and theme.child_type1 classes. But passed theme object can contain multiple concatenated classnames on these keys so to include everything I have to concat manually.

Although I agree that it's a bit verbose but is much more clear and consise because it's explicit. Morever it provides dynamic theme construction depending on params.

[vectorsize]

looks good to me, I will try to find time to build a replica of my use-case so if the same performance issues are still there we can all take a look at it @raveclassic @denisborovikov and @javivelasco

[javivelasco]

Alright, meanwhile and seeing the transpile I'm going to merge. Thank you all! 👍