Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order of properties can change in different environments #199

Closed
lencioni opened this issue Mar 2, 2017 · 13 comments · Fixed by #200
Closed

Order of properties can change in different environments #199

lencioni opened this issue Mar 2, 2017 · 13 comments · Fixed by #200

Comments

@lencioni
Copy link
Collaborator

lencioni commented Mar 2, 2017

I am noticing a difference in how the style properties are ordered between a development environment and a production environment. Essentially, I have a couple of styles that look like this:

foo: {
  margin: 0,
},

bar: {
  marginRight: 16,
},

I am applying them to my element like this css(foo, isBar && bar).

In my development environment (a story in react storybook), the component is styled as expected, with 0 margin all around except for the 16px of margin on the right. However, in production, my 16px right margin is missing. Looking at the generated styles, I can see that all of my properties are there, but the order is different.

In storybook:

.an_aphrodite_selector {
  margin: 0 !important;
  margin-right: 16px !important;
}

In production:

.an_aphrodite_selector {
  margin-right: 16px !important;
  margin: 0 !important;
}

I also have some more styles than just margin, but I am just mentioning these because it highlights the specific problem. Here's the specific CSS that is generated in development (whitespace added for readability):

.text_1aunhvg-o_O-tab_8ndy7u-o_O-tab_micro_17dr3ug-o_O-text_muted_td6kew-o_O-text_spacedTabs_1fn1i59-o_O-tab_selected_6lr1xw{
	-webkit-transition-timing-function:ease-out !important;
	-webkit-transition-property:color !important;
	-webkit-transition-duration:300ms !important;
	letter-spacing:0.4px !important;
	padding-top:12px !important;
	padding-bottom:12px !important;
	color:#008489 !important;
	margin:0px !important;
	background-color:transparent !important;
	border-width:0px !important;
	border-bottom:2px solid #008489 !important;
	font-size:12px !important;
	cursor:pointer !important;
	margin-right:16px !important;
	padding-left:0px !important;
	padding-right:0px !important;
	font-weight:700 !important;
	transition-property:color !important;
	transition-duration:300ms !important;
	transition-timing-function:ease-out !important;
	font-family:Circular,-apple-system,BlinkMacSystemFont,Roboto,Helvetica Neue,sans-serif !important;
	line-height:16px !important;
	bottom:-1px !important;
}

and in production:

.text_1aunhvg-o_O-tab_8ndy7u-o_O-tab_micro_ltghyi-o_O-text_muted_td6kew-o_O-text_spacedTabs_1fn1i59-o_O-tab_selected_6lr1xw{
	padding-left:0px !important;
	bottom:-1px !important;
	font-weight:700 !important;
	border-width:0px !important;
	border-bottom:2px solid #008489 !important;
	cursor:pointer !important;
	padding-top:12px !important;
	font-size:12px !important;
	letter-spacing:0.4px !important;
	font-family:Circular,-apple-system,BlinkMacSystemFont,Roboto,Helvetica Neue,sans-serif !important;
	margin-right:16px !important;
	padding-right:0px !important;
	padding-bottom:18px !important;
	line-height:16px !important;
	color:#008489 !important;
	margin:0px !important;
	background-color:transparent !important;
	transition-property:color !important;
	transition-duration:300ms !important;
	transition-timing-function:ease-out !important;
	-webkit-transition-property:color !important;
	-webkit-transition-duration:300ms !important;
	-webkit-transition-timing-function:ease-out !important;
}

Any ideas what might be happening here or where to look? It seems that there might be some code that expects the order of things to be stable when that is not the case. Or perhaps we are transforming (Babel) or polyfilling something slightly differently in these two environments?

Aphrodite v0.5.0

@lencioni
Copy link
Collaborator Author

lencioni commented Mar 2, 2017

Also, I noticed that the generated selector is the same, which I believe implies that the styles are passed to Aphrodite in the same order in both environments.

@lencioni
Copy link
Collaborator Author

lencioni commented Mar 2, 2017

I'm thinking that anywhere object keys are used, they will need to be manually sorted to preserve developer intention and consistency across environments and browsers.

@ljharb
Copy link

ljharb commented Mar 2, 2017

Note that this includes any usage of Object.keys/Object.entries/Object.values, Object.getOwnPropertyNames, and for..in.

@xymostech
Copy link
Contributor

@lencioni Huh! That's very... annoying. I'd love to get some more information about the two environments you're rendering the different styles in. Are you server-side rendering in one, or is it two different browsers?

Also, could you try updating to the current version of Aphrodite (v1.1.0) and see if you're still experiencing the problem?

@lencioni
Copy link
Collaborator Author

lencioni commented Mar 2, 2017

Ah, yes, in production we are server side rendering. That is almost certainly the key difference here.

I'll work on updating Aphrodite, but it might take me a while.

@ljharb
Copy link

ljharb commented Mar 2, 2017

Property key ordering also changed noticeably in a specific version of v8, which means node-to-chrome across that boundary, or node-to-nonv8, will have this problem.

@xymostech
Copy link
Contributor

Okay. I've feel like I've seen that happen before also when server-side rendering :(. Do you know what version of node you're using on your server? And what browser are you using? I haven't seen it in a while, so maybe some combination of node/aphrodite upgrade fixed it, or maybe we're just not triggering it any more.

@ljharb Huh, interesting! I didn't know that it had changed. Do you know when that happened, or can you find docs about it?

@ljharb
Copy link

ljharb commented Mar 2, 2017

@xymostech object property ordering became specified in ES6, so all engines will eventually have consistent ordering, but it'll always be a good practice to not rely on key ordering.

We were using node 4.2.4 and now 4.6.2.

@lencioni
Copy link
Collaborator Author

lencioni commented Mar 2, 2017

I think it is important to stabilize the sorting of these styles, but it might also be interesting to explore a more advanced merging strategy for shorthand properties. e.g. merge margin: 0 and marginRight: 16 to be `margin: '0 16px 0 0'.

@ljharb
Copy link

ljharb commented Mar 2, 2017

Indeed, I think both would be a good idea.

@xymostech
Copy link
Contributor

@lencioni while that sounds interesting, I think I'd like to avoid doing merging into the shorthand properties like that because it would probably involve understanding a lot of CSS semantics, which we're trying to avoid in Aphrodite.

Going the other way seems more interesting to me, though. If you specified margin-left: 0; margin-right: 0; margin-top: 0; margin-bottom: 0; instead of just margin: 0, this wouldn't have been a problem, correct? Maybe if we added like a margin mixin so you wouldn't have to write all of the properties out yourself?

StyleSheet.create({
    foo: {
        ...margin(0),
    },
    bar: {
        marginLeft: 15,
    },
})

@xymostech
Copy link
Contributor

I'm thinking that anywhere object keys are used, they will need to be manually sorted to preserve developer intention and consistency across environments and browsers.

I'm looking into doing this sorting, and I'm not really sure what this means. Just plain sorting the properties obviously isn't what we want, because then margin might end up after margin-left or something. But if the problem is object ordering, how do we pull out the intended order from the objects that are passed in? Hmm.

@ljharb
Copy link

ljharb commented Mar 2, 2017

Right, exactly. If the user provides the keys in object form, there's no way to know what sorting they intended.

A better approach would be if the user provided a Map, or an array of entries ([key, value]), since passing an object doesn't convey any order.

xymostech added a commit that referenced this issue Mar 3, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 3, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 3, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 7, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 7, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 8, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 8, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 8, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
xymostech added a commit that referenced this issue Mar 8, 2017
Summary: Key ordering in objects can be different in different environments.
Sometimes, this causes problems, like where styles generated on the server are
not in the same order as the same styles generated on the client. This
manifests in problems such as #199

This change lets users manually fix instances where the ordering of elements
changes by specifying their styles in an ES6 `Map`, which has defined value
ordering.

In order to accomplish this, an `OrderedElements` class was created, which is
sorta like a `Map` but can only store string keys and lacks most of the
features. Internally, `Map`s and objects are converted into this and then these
are merged together to preserve the ordering.

Fixes #199

Test Plan:
 - `npm test`

@lencioni @ljharb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants