[Core] Implements prepareStyles as composition of functions in muiTheme #2986

newoga · 2016-01-20T00:06:16Z

This commit implements the prepareStyles function as a composition of functions that can be configured in muiTheme. It switches the ./mixin/style-propable.js implementation to use the prepareStyles stored in context.

@oliviertassinari @alitaheri
This is an initial implementation of the approach of storing prepareStyles in context. I cleaned up the code a bit, however like I said, I don't think we should document this until we've confirmed we're happy and the API is final (I already have some ideas of how to improve it, but it shouldn't have any real immediate benefit). It's also very well possible this will all go away when we adopt a new styling framework.

Note: One minor difference I made is that the getMuiTheme function actually checks the computed theme for isRtl before composing the functions. If it's false, it doesn't even bother including the ensureDirections() (renamed to rtl()) method. Edit: It now does this for the autoprefixer() method based on the userAgent property

Right now, the list of transformers can be overridden by adding the following property to the muiTheme. The following configuration would essentially disable the default behavior of always autoprefixing and calling rtl() when isRtl is equal to true. Removed customizing of transformers in this PR, other approaches have since been implemented.

Remaining tasks:

Move implementation of "double call warning" out of rtl() into something that runs before all style transformations are executed. Done
~~Change rtl() implementation to not care is isRtl~~ No longer necessary

Future PR:

Convert components not using ./mixins/style-propable.js to get prepareStyles() from context, not from importing it from ./utils/style.js.
Deprecate and/or remove ./utils/style.js

newoga · 2016-01-20T00:17:20Z

Related to #2316, #2530, #2852.

alitaheri · 2016-01-20T06:31:51Z

@newoga Awesome work indeed 👍 👍 Thank you 😄

alitaheri · 2016-01-20T06:33:30Z

src/compose.js

@@ -0,0 +1,3 @@
+import flowRight from 'lodash.flowright';


Do we need these 2 files? I mean it's gonna look like "API surface" to me. We can just require these whenever we want. and I'm not seeing a day we wouldn't use lodash, you know, since it's awesome 😁

@alitaheri I had the same though and I'm open to removing these. The only library I thought we may one day replace these with is recompose, which ironically also uses lodash's compose for its implementation.

Either way I'm going to take it out compose.js until we start using it in several places.

I wanted to talk to you two about merge.js though. I'm thinking we should replace all mergeStyles with one of the three:

merge

Object.assign

destructuring

how about lodash.assign? It uses Object.assign if it's available. and it's faster than extend or merge imo. @oliviertassinari Thoughts?

For the record I'd be happy with that 👍

alitaheri · 2016-01-20T20:28:31Z

I believe we should use assign for every style merging too. 👍 Thanks @newoga

oliviertassinari · 2016-01-21T11:17:18Z

src/styles/transformers/rtl.js

+  }
+
+  // Left to right is the default. No need to flip anything.
+  if (!muiTheme || !muiTheme.isRtl) return style;


I think that we don't need this line.
More generally, do we really need to provide the muiTheme to the transformers?

@oliviertassinari Let's not limit ourselves for now. We might want to add other things, or if this is gonna be documented, others might like to have access to the theme in their transformers. the impacts of having it is likely none. but excluding it completely might make it hard to put back (or maybe even impossible). besides! we might even be able to use this to pass down userAgent down the context and use it in the prefixer. Just saying 😁

@oliviertassinari I agree we don't need the line but also agree with @alitaheri that we should possibly leave the muiTheme in the transformers for now.

The reason why the muiTheme was there in the first place is because this ensureDirections now names (rtl) was left untouched minus the name and merge implementation. I believe the rtl check could be removed and the warning show be moved top level too (these are currently listed as remaining task at the top of the PR)

newoga · 2016-01-24T07:05:23Z

@oliviertassinari @alitaheri

I made some additional changes to this and rebased onto master so that it includes @oliviertassinari's auto-prefix changes. I decided to take passing in custom transformers out of the API for now. I think having that feature is now less urgent because the the two existing transformers (prefixing and RTL) can be disabled through muiTheme configuration (userAgent set to false and isRtl set to false disables both).

If we want to explore making this publicly accessible, I think we can explore options in a new PR.

The two key changes I made were the following:

Moved the check for running `prepareStyle()` twice out of the `rtl()` function

I decided to not to make the check a transformer that was part of the chain, because there would be no way to cancel the rest of the chain from running. I decided instead to wrap the chained function with the check so that it gets called first and exits if it sees if it was ran already.

In addition, I changed the check from using a object key to a Symbol. I think this is safer so that developers can't set the property to "trick" the check into running and it also gets the nasty did-flip keys out of the style objects in the DOM.

The end result is that this check is ran in development AND production, just that it only logs a warning in development. I think this is the better approach though.

Changed `mergeStyles()` implementation to `Object.assign()`.

The first reason is because I think it's simpler, but also Object.assign() supports assigning Symbol keys and out previous merge implementation did not. This change was required for the first step above. I think we were all leaning towards moving towards either Object.assign() or lodash's assign anyway.

alitaheri · 2016-01-24T07:32:25Z

@newoga I see you have brainstormed the crap out of it 👍 👍 Really great job. I really like your approaches 👍 👍 🎉

Also, Does the polyfill also assign Symbols?

alitaheri · 2016-01-24T07:36:10Z

src/utils/styles.js

-  if (!objB) return objA;
-  return update(objA, {$merge: objB});
-}
+export const mergeStyles = (...args) => Object.assign({}, ...args);


Do we really need to clone? I think it's an unnecessary allocation of an extra object. @oliviertassinari Thoughts?

Debatable though. I understand immutability, but since this is part of a chain, maybe it won't be all that necessary. Although I think the implications of cloning are a lot less than not. But still...

I wish we didn't need this but it was breaking things when we weren't cloning. I think we have some situations in source where we may be passing in an undefined style, but I haven't been able to pinpoint everywhere it's happening yet. I just know calling simply Object.assign broke the docs site and the tests.

I think it's worth going back and investigating though! I think the ideal would be that we shouldn't need to do this.

Oh and one more point. the mergeStyles is really going to be deprecated/removed when style-propable is removed. This is kind of temporary.

I'm recommending we just reimplement the components to use the prepareStyles from context directly, not from the style-propable mixin, and anywhere we were calling mergeStyles() we replace with Object.assign. After that this can go away.

Well, not having it is dangerous. Let it be, With memoization and proper optimization this doesn't matter at. So we better keep it like this.

I agree, sounds good 👍

I'm recommending we just reimplement the components to use the prepareStyles from context directly, not from the style-propable mixin, and anywhere we were calling mergeStyles() we replace with Object.assign. After that this can go away.

Oh, right. So all the more reason to ignore what I said :D :D This is great as it is 😁

Do we really need to clone?

I think that we don't have the choice regarding how we use the merge method in the source code.
I guess we could work on it but that out of the scope of this PR. It's perfect like this for now 👍.

newoga · 2016-01-24T07:55:22Z

Also, Does the polyfill also assign Symbols?

That's an intersting question. I assumed so but assuming can be dangerous 😛. I'll have to investigate this more...

alitaheri · 2016-01-24T08:16:32Z

This is the polyfill. unfortunately it doesn't support Symbols. but I think this wouldn't matter because if Object.assign is not supported then so isn't Symbol. They both are parts of the same version 😆 😆

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

oliviertassinari · 2016-01-24T20:51:23Z

src/styles/getMuiTheme.js

+import compose from 'lodash.flowright';
+import warning from 'warning';
+
+const CALLED_ONCE = Symbol('prepareStyles/calledOnce');


Introducing a Symbol will have some implication, have a look at the issues that this PR solve #2904.
I think that it's a breaking change.

Mm, interesting. What do you and @alitaheri prefer? Use a regular string key or polyfill?

I wonder we're inevitably going to keep running into Symbol issues because developers often assume it's "included".

I think we should avoid polyfills for the time being. es6 has to become mainstream before we can use it libraries. I use every feature i can get my hands on in my application. but libraries are different 😁

Okay did-flip from before made more sense in the context of ensureDirection. Do you have a preference to what key we use? prepared: true?

What about mui-prepared: true?
We should consider using a Symbol again with v0.15.0.

newoga · 2016-01-24T22:31:14Z

@oliviertassinari @alitaheri Alright guys, I think we're close 😣

This has the following changes:

Moved all the "transformer inclusion" checks into the transformer definitions themselves. They will be filtered out if they don't return a truthy value.
Since we decided that there are performance implications with the callOnce, I moved it out to a transformer like @oliviertassinari originally suggested. It returns early and is filtered out if it's in aaproduction environment.
Replaced the Symbol with just a string for the callOnce check (to avoid polyfill).

oliviertassinari · 2016-01-24T22:42:33Z

@newoga I think we have it! I like the current approach 👍. We will have to refactor a lot of code once it's merged.

newoga · 2016-02-01T02:56:50Z

@oliviertassinari @alitaheri

FYI just rebased onto master so this didn't get too far behind. I also decided to remove the re-implementation/migration of mergeStyles to Object.assign from this PR. The feature presented in this PR (composing style transformers) works just fine with the existing merge implementation. I still think we should switch to Object.assign but I decided to present that in a new PR #3124 so we can finish that discussion there.

I think both PRs will be easier to understand when they're separated and these changes don't rely on one another so they don't need to be merged together.

oliviertassinari · 2016-02-01T19:05:26Z

src/styles/transformers/callOnce.js

+const CALLED_ONCE = 'muiPrepared';
+
+export default function callOnce() {
+  if (process.env.NODE_ENV === 'production') return;


What about englobing the all code inside the if statement?
I'm not sure uglify is smart enough to remove unreachable code.

oliviertassinari · 2016-02-01T19:09:01Z

@newoga Yes, let's merge this 🎉

newoga · 2016-02-01T19:14:14Z

@oliviertassinari Sounds good! @alitaheri Do you want to give this a last look?

oliviertassinari · 2016-02-01T19:15:33Z

@alitaheri or @mbrookes feel free to merge if you are happy with it.

alitaheri · 2016-02-01T19:21:31Z

All good and awesome 👍 👍 JUst fix ~~those 2 things~~ that one thing pointed out by @oliviertassinari (unreachable-code) and do the merge. Really nice work @newoga 👍 👍

Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>

newoga · 2016-02-01T19:51:11Z

Thanks @alitaheri, much appreciated 😄

If all is green, this should be good to go.

[Core] Implements prepareStyles as composition of functions in muiTheme

oliviertassinari · 2016-02-01T20:52:12Z

@newoga Awesome work, 133 messages in this PR 🚀!

newoga · 2016-02-01T21:03:37Z

I know what a long one! Thanks guys! Big group effort 🎉 🎈

alitaheri · 2016-02-01T21:28:38Z

Really nice work. this was a big step indeed 👍 👍

The commit allows us to start using `Object.assign` for merging styles instead of this `mergeStyles` utility method. Chrome's currently has a bug in which its `Object.assign` implementation does not consistently or keys of merged objects. This sometimes led to styling bugs because some style properties rendered to the DOM in the wrong order (see mui#2986). This commit includes a babel transformation step that replaces all uses of `Object.assign` with the implementation at `simple-assign` and ignores the browser's native Object.assign implementation. Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>

newoga added PR: Needs Review labels Jan 20, 2016

alitaheri reviewed Jan 20, 2016
View reviewed changes

oliviertassinari reviewed Jan 21, 2016
View reviewed changes

newoga mentioned this pull request Jan 22, 2016

[Style] Address server sider rendering issues #3009

Merged

newoga force-pushed the style-refactor-experiment branch 3 times, most recently from 7a409c9 to dbb7bb5 Compare January 24, 2016 06:54

alitaheri reviewed Jan 24, 2016
View reviewed changes

oliviertassinari reviewed Jan 24, 2016
View reviewed changes

newoga force-pushed the style-refactor-experiment branch from dbb7bb5 to 4c3d1e0 Compare January 24, 2016 22:22

newoga changed the title ~~Implements prepareStyles as composition of functions in muiTheme~~ [Core] Implements prepareStyles as composition of functions in muiTheme Feb 1, 2016

newoga mentioned this pull request Feb 1, 2016

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

Merged

newoga force-pushed the style-refactor-experiment branch from a704248 to bd8d6ba Compare February 1, 2016 02:54

oliviertassinari reviewed Feb 1, 2016
View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Feb 1, 2016

Implements prepareStyles as composition of functions in muiTheme

2f0d184

Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>

newoga force-pushed the style-refactor-experiment branch from bd8d6ba to 2f0d184 Compare February 1, 2016 19:47

oliviertassinari added a commit that referenced this pull request Feb 1, 2016

Merge pull request #2986 from newoga/style-refactor-experiment

04b7c23

[Core] Implements prepareStyles as composition of functions in muiTheme

oliviertassinari merged commit 04b7c23 into mui:master Feb 1, 2016

newoga deleted the style-refactor-experiment branch February 6, 2016 23:28

newoga mentioned this pull request Apr 12, 2016

[core] Remove Object.assign replacement of simple-assign #3948

Closed

oliviertassinari mentioned this pull request Apr 15, 2016

React 15+ inline styling is producing concatenated style properties from separate, unique object keys facebook/react#6524

Closed

oliviertassinari mentioned this pull request Dec 5, 2020

[core] Replace fs-extra deprecated function (exists) #23848

Merged

1 task

zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023

[Core] Implements prepareStyles as composition of functions in muiTheme #2986

[Core] Implements prepareStyles as composition of functions in muiTheme #2986

Conversation

newoga commented Jan 20, 2016

newoga commented Jan 20, 2016

alitaheri commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Jan 24, 2016

Moved the check for running prepareStyle() twice out of the rtl() function

Changed mergeStyles() implementation to Object.assign().

alitaheri commented Jan 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Jan 24, 2016

alitaheri commented Jan 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newoga commented Jan 24, 2016

oliviertassinari commented Jan 24, 2016

newoga commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Feb 1, 2016

newoga commented Feb 1, 2016

oliviertassinari commented Feb 1, 2016

alitaheri commented Feb 1, 2016

newoga commented Feb 1, 2016

oliviertassinari commented Feb 1, 2016

newoga commented Feb 1, 2016

alitaheri commented Feb 1, 2016

Moved the check for running `prepareStyle()` twice out of the `rtl()` function

Changed `mergeStyles()` implementation to `Object.assign()`.