[AutoPrefix] Change getTrasform() to return the prefixer object

[felipethome]

I believe there are three problems with the AutoPrefix module:

1. The getTransform() method is returning the prefix() method from the Prefixer class. This will not work since inline-style-prefixer has statements like this one:

   if (!this._hasPropsRequiringPrefix) {
     return styles
   }
   

   When one calls muiTheme.prefix() he is actually setting the this keyword to the muiTheme object (so, _this.hasPropsRequiringPrefix would be undefined). This is a big problem because it means that none of the styles transformations being called with set() is working.

2. The set() method has the signature: set(style, key, value, muiTheme). Then, inside of it there are some piece of code like this:

   style = muiTheme.prefix(style);
   

   This will not work because it is being passed a reference to the style object, so when you assign a new value to it we are actually loosing the reference to the original object and not changing anything at all in the caller.

3. As a setter the set function should not pass the entire style object to the prefix method, because inside of its code it has:

   Object.keys(styles).forEach(property => {...}
   

   So prefix() will iterate though all the style properties.

Obs: in the all() method inside the AutoPrefix module there are statements returning the object created by InlineStylePrefixerInstance.prefix(). That approach would not work for the set method because material-ui has statements like this one:

let style = ReactDOM.findDOMNode(this).style;

The style object returned by a DOM element is actually a CSSStyleDeclaration and not a conventional Javascript object so one definetelly can not override it with the return object from InlineStylePrefixerInstance.prefix().

[newoga]

@felipethome Looking at this from a high level the changes seem correct and reasonable. The interesting thing is pretty much all these methods (except for getTransform) are going to be removed next version in this PR.

That being said:

The getTransform() method is returning the prefix() method from the Prefixer class. This will not work since inline-style-prefixer has statements like this one:

That makes sense I think your changes in this area make sense and I think we should change this.

Though the set method isn't used in a ton of places in the code and the plan is to migrate all the components that use that method to something else.

@oliviertassinari @alitaheri @mbrookes How do you think we should handle this in light of preparing next version?

[felipethome]

In the PR you mentioned it seems the set method will remain. It will be shorter, but still with the reference problem.

[newoga]

In the PR you mentioned it seems the set method will remain. It will be shorter, but still with the reference problem.

Yeah I know, we still plan to remove it 😄 We just haven't change the components that use it to use not use it yet.

[alitaheri]

From what I understood from the issues you mentioned. It has a reference issue, wouldn't

return (style) => prefixer.prefix(style);

fix it?

[felipethome]

True, I believe it would. And it is actually better like that since I forgot to change this other code:

if (userAgent === false) { // Disabled autoprefixer
  return (style) => style;
} else if (userAgent === 'all' || userAgent === undefined) { // Prefix for all user agent
  return InlineStylePrefixer.prefixAll;
...

Now, what about the set() method? It will be removed but the components that use this method are not having their styles transformed right now.

[newoga]

return (style) => prefixer.prefix(style);

That makes sense to me.

Now, what about the set() method? It will be removed but the components that use this method are not having their styles transformed right now.

I think the easiest way to deal with this once an for all is to migrate the components that are using set() to use the prefixAll method now included in muiTheme.

[oliviertassinari]

@felipethome That's a really good finding 🎉! The auto-prefixer just doesn't work 😨.
Could you use the fix proposed by @alitaheri?

return (style) => prefixer.prefix(style);

What about adding a test so that this type of regression never happen again 🙈?

That's some very good observations we plan to remove the set method.
The all is already deprecated.

This will not work because it is being passed a reference to the style object, so when you assign a new value to it we are actually loosing the reference to the original object and not changing anything at all in the caller.

Well, that explains why when I tried to remove the set method, I was breaking the CircularProgress.
I was applying the prefixes, prefixes that the component doesn't like.
I feel like we could simply the set method with the following code for now... 😁

  set(style, key, value) {
    style[key] = value;
  }

[felipethome]

@oliviertassinari Of course, I will do it . And thanks @alitaheri for the insight.
So, maybe we should dig in what is happening with the CircularProgress? It shouldn't break when applying the prefixes. And soon this prefixes will be applied anyway with the prefixAll method that @newoga was talking about right?

The test you are purposing should test just the getTransform method?