[Core] Replace merge implementation in utils/styles with Object.assign

[newoga]

This PR splits of the some of the work that was started in #2986 that switched our merge implementation to Object.assign.

The essence of this PR is to pave the way to start using Object.assign for merging styles instead of a merge utility method. The problem we ran into was that Chrome's Object.assign implementation currently has a bug in which the keys were not always ordered properly (which sometimes led to styling bugs because of the order style properties were defined). This PR includes a babel transformation step (that needs to be reviewed) that uses the same implementation as babel-plugin-transform-object-assign from the babel repository, except it will not use the native Object.assign when found.

Note: This does not replace browser's Object.assign, it simply uses a different method (similar to a ponyfill).

[newoga]

If we continue to pursue this PR, we should probably tag release rather than work of master at the very least and possibly publish to npm too. Though just wanted to run this implementation by you first (and confirm what we should name these).

[oliviertassinari]

Same comment than for the babel plugin.

[newoga]

Yeah originally I wanted to design the plugin to create a file with the right implementation in the build output, but I couldn't figure out how to do it and I didn't see an easy way to resolve relative paths for components in babel, so making it a package with a static import name was easiest.

[alitaheri]

This looks good to me, great job with the transformer 👍 👍

You say it doesn't replace the browsers Object.assign. But didn't Chrome have an issue with it? isn't that why we weren't using _extend?

[newoga]

You say it doesn't replace the browsers Object.assign. But didn't Chrome have an issue with it? isn't that why we weren't using _extend?

Yes kind of, both _extend and this transform technically replaces all your uses of Object.assign with another method that replicates or copies the Object.assign implementation. The only difference between this transform and _extend is if Object.assign is found in the browser, it isn't used. Small difference! 😄

[alitaheri]

Oh, right my bad. I misunderstood 😅

[newoga]

Oh, right my bad. I misunderstood 😅

No problem, I wrote it in a really confusing way. 😄

@oliviertassinari
I was able to make a change to my babel plugin that fixes the issue about using Object.assign multiple times. I was able to test it and it looks like this approach is working.

[oliviertassinari]

It would be good to publish it. If it's just designed to be used with material-ui and won't help the comminity, we can even consider moving the source code to this repository.

[newoga]

Do you have a preference as to what we call it?
babel-plugin-transform-inject-object-assign
babel-plugin-transform-replace-object-assign
{any other ideas}

[oliviertassinari]

babel-plugin-transform-replace-object-assign 👍

[newoga]

Sounds good.

[newoga]

@oliviertassinari @alitaheri FYI - I published the plugin and assign implementation to npm and switched this to pull from there instead of github. 😄

[alitaheri]

Beautiful work @newoga 👍 👍 😍

[newoga]

Beautiful work @newoga 👍 👍 😍

Thanks! We're making a lot of progress! 🎉

[oliviertassinari]

@newoga Nice job! Can we rebase and squash down the commit? Let's merge this 🎉 🚀.

[oliviertassinari]

I have checked the babel-plugin-transform-replace-object-assign plugin
and npm run browser:prd is working fine 👍 .
It's transforming

export const mergeStyles = (...args) => Object.assign({}, ...args);
export const mergeStyles2 = (...args) => Object.assign({}, ...args);

into

var _simpleAssign = require('simple-assign');

var _simpleAssign2 = _interopRequireDefault(_simpleAssign);

var mergeStyles = exports.mergeStyles = function mergeStyles() {
  for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key++) {
    args[_key] = arguments[_key];
  }

  return _simpleAssign2.default.apply(undefined, [{}].concat(args));
};
var mergeStyles2 = exports.mergeStyles2 = function mergeStyles2() {
  for (var _len2 = arguments.length, args = Array(_len2), _key2 = 0; _key2 < _len2; _key2++) {
    args[_key2] = arguments[_key2];
  }

  return _simpleAssign2.default.apply(undefined, [{}].concat(args));
};

but it would be good to add some tests inside https://github.com/newoga/babel-plugin-transform-replace-object-assign.

[newoga]

but it would be good to add some tests inside https://github.com/newoga/babel-plugin-transform-replace-object-assign.

I agree, @oliviertassinari. I was thinking the same thing when I was writing it, but didn't get around to looking into good test approaches for babel plugins. I just saw how you did it here. I'll use that approach to check for actual and expected for what you showed above, and also make sure there is a test for multiple uses of Object.assign which I fixed. I'll try to get that done tonight.

I rebased this branch for now. If we feel comfortable enough merging this as is, it should be good to go, otherwise later today after the tests is fine too!

[oliviertassinari]

If we feel comfortable enough merging this as is, it should be good to go, otherwise later today after the tests is fine too!

I would say, let's wait for the tests 👍😁.
You can also use https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/tree/master/test/fixtures as a source of inspiration.

[newoga]

Sounds good!

You can also use https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/tree/master/test/fixtures as a source of inspiration.

Thanks for sharing, that's a great reference.

[newoga]

@oliviertassinari Thanks for letting me use your babel plugin as a guide for making mine! I've added tests and setup the build in travis-ci!

Let me know if there are any additional scenarios that are worth testing for. No changes were made to src, but I still made a patch release anyway and updated the dependency here to be sure.

[oliviertassinari]

@newoga Awesome! Thanks 💯