merge is one-level

[gutenye]

Given

const s = StyleSheet.create({                                                   
  red: { 
    '&.active': {                                                               
      width: '100px',                                                           
    }                                                                           
  },                                                                            
  blue: {
    '&.active': {                                                               
      height: '100px',                                                          
    }                                                                           
  },                                                                            
})  

<div className={cx(css(s.red, s.blue), 'active')}>                 

Got

.red-3089191041--blue-1941479325.active {
  height: 100px;
}

Expect

.red-3089191041--blue-1941479325.active {
  width: 100px;
  height: 100px;
}

[kof]

Yeah so you expected a deep merge there, but got a shallow merge.

[kof]

Can be done. Do you like to send a pr?

[gutenye]

I haven't read the source code yet.

[kof]

Not sure though if I want a library for deep merge or just write own simpler version, because libraries handle tons of cases we don't have.

[asvetliakov]

@kof Will you accept PR for this using lodash.merge ?

[kof]

I am not sure its a good idea, in our case here we know what kind of objects we accept, its not as generic as lodash. I guess it will come with a big amount of utility functions and increase the build size a lot.

[kof]

Just installed lodash.merge, it comes with 2200LOC (16kb minified not gzipped), covering all the cases we don't have: array like objects, typed arrays, isPlainObject check (we always have plain object), arguments etc etc.

[kof]

https://www.npmjs.com/package/deep-assign and https://www.npmjs.com/package/deepmerge seem to be better options in our case, I would go for the first one, because I know @sindresorhus makes very high code quality.

[kof]

Btw. the version aphrodite uses is even smaller, because more specific to supported cases: https://github.com/Khan/aphrodite/blob/master/src/util.js#L40

[asvetliakov]

Yes, lodash.merge it's pretty big. 60Kb
what about simplier version then (not tested)?:

function recursiveMerge(val1 = {}, val2 = {}) {
    for (const key of Object.keys(val2)) {
        if (val2.hasOwnProperty(key) {
          val1[key] = (typeof val2[key] === "object" && val2[key] != null) ? recursiveMerge(val1[key],   val2[key]) : val2[key];
      }
    }
    return val1;
}

const finalStyle = rules.map(rule => rule.style).reduce(recurisveMerge, {});

[sindresorhus]

Don't use deep-assign. It's broken and I haven't had time to fix it up. I would also not recommend creating your own version. It's very hard to get it right. Lots of edge-cases. Just use a Lodash method for this. It's high-quality.

[kof]

@sindresorhus thanks for heads up, we have a very predictable use case here, not a generic one. We have only a need to support plain objects and arrays. Basically only JSON subset, no regexp, no arguments etc., what would you do?

[kof]

I think we should go for a custom merge for our case.

[kof]

@gutenye any updates?

[gutenye]

The discussion is very helpful. But I haven't started working on it (don't have much time recently). You can close this issue if you want.

…

On Mon, May 22, 2017 at 3:30 PM, Oleg Slobodskoi ***@***.***> wrote: @gutenye <https://github.com/gutenye> any updates? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXCyBvsKiO7Sq9HcjBvUnIa4khYs92Tks5r8Tl5gaJpZM4K7twQ> .

-- JavaScript, Go, Linux and Open Source - g <http://GutenYe.com>uten.me - github.com/g <http://Github.com/GutenYe>utenye

[SamyPesse]

I've published an alternative to this module: https://github.com/SamyPesse/aphrodite-to-jss that is more compatible with Aphrodite and supports the recursive merging.

@kof I didn't wanted to do it as a PR on this module as it'd have been be a breaking change with the styles supported by aphrodite-jss, and those changes were required to switch the code base of GitBook to use JSS as a CSSinJSS backend.

If you think it makes sense to have those changes in aphrodite-jss, I'll be happy to help (and remove the need for my module) :)

[kof]

aphrodite-jss is first of all a compatible adapter for aphrodite, so it should be as close as possible to what aphrodite does. I am not using aphrodite, so I am not aware of the details. I think we should release a major version if its a breaking change.

[SamyPesse]

The main differences we spotted when switching our code base to JSS (hundreds of components):

• Animation keyframes: Aphrodite supports assigning keyframes to animationName: animationName: { from: { opacity: 0 }, to: { opacity: 1 } }
• Syntax for pseudo elements: Aphrodite uses ':hover': { } while JSS uses '&:hover': {}
• SSR autoprefixing
• Array fallback: Aphrodite supports using array to define fallbacks background: ['red', 'linear-gradient(to right, red 0%, green 100%)'] while JSS uses a fallback property
• Font face: Aphrodite supports assigning a font-face directly as fontFamily
• And the most import: merge of styles when calling css(a,b,c,[d]): Aphrodite merges the properties and fallback

None of this is supported by aphrodite-jss at the moment (it is compatible with the API, not with the styles syntax).

I've done those changes in aphrodite-to-jss because it was impossible for us to switch all of our styles to the JSS syntax. My idea is to print warning for Aphrodite styles and progressively switch to the JSS syntax.

[kof]

Animation keyframes: Aphrodite supports assigning keyframes to animationName: animationName: { from: { opacity: 0 }, to: { opacity: 1 } }

Can be added as part of aphrodite-jss transformation and converted to JSS format. Alternatively it could be implemented as a JSS plugin. I was thinking for quite some time about a good syntax for keyframes and we ended up with this in the latest v10 alpha. The reason we didn't go with aphridite's syntax is that semantically animationName is a name, but instead aphrodite expects the keyframes.

[kof]

Syntax for pseudo elements: Aphrodite uses ':hover': { } while JSS uses '&:hover': {}

Yes, & is syntax for nesting that also additionally supports pseudo selectors. We have a long standing issue to add a plugin for using pseudos without & cssinjs/jss#334 as a separate plugin, now with monorepo, adding plugins is easy, I think it should be also part of the default preset.

[kof]

SSR autoprefixing

Also a long standing issue, JSS vendor prefixer was always optimized for bundle size thats why we also went for feature detection at runtime, so it is currently incapable of server-side prefixing.

I think we should add the missing parts for SSR to jss-plugin-vendor-prefixer or add inline-style-prefixer for SSR to it. cc @AleshaOleg (who was mainly focused on prefixer lately).

[kof]

Array fallback: Aphrodite supports using array to define fallbacks background: ['red', 'linear-gradient(to right, red 0%, green 100%)'] while JSS uses a fallback property

How hard would be migrating from this syntax? JSS has a different fallbacks syntax, because we wanted to use arrays for multi-part values.

[kof]

Font face: Aphrodite supports assigning a font-face directly as fontFamily

Similar to keyframes, this can be implemented as part of the transformation or as a plugin. JSS went with this syntax, because of better semantics.

Another thing: font-faces are used rarely, it is unlikely a problem to migrate, so not sure it is worth the effort.

[kof]

And the most import: merge of styles when calling css(a,b,c,[d]): Aphrodite merges the properties and fallback

Yep, this can be done here.

[kof]

@SamyPesse I just made you admin of this repo and added you as owner of the npm package, so that I don't block you and you can update this package directly.

[kof]

Regarding the above changes in JSS repo, feel free to send a PR!

[SamyPesse]

pseudo elements: Yes, & is syntax for nesting that also additionally supports pseudo selectors.

Great, I like the JSS syntax, but having a safe migration path is better in our case.

---

Array fallback: How hard would be migrating from this syntax? JSS has a different fallbacks syntax, because we wanted to use arrays for multi-part values.

Quite easy I think, I don't any part where we use it, it's mostly supported by Aphrodite because of inline-style-prefixer (which replace display: 'flex' by display: ['-webkit-flex', ... , 'flex'].

But it can be handled by the prefixer plugin without being a global support.

---

SSR autoprefixing: I think we should add the missing parts for SSR to jss-plugin-vendor-prefixer or add inline-style-prefixer for SSR to it. cc @AleshaOleg (who was mainly focused on prefixer lately).

This is done in aphrodite-to-jss using inline-style-prefixer. Basically I have one module that transform "Aphrodite and/or JSS styles" to J JSS styles: https://github.com/SamyPesse/aphrodite-to-jss/blob/master/src/normalize.ts

---

Font-face: Another thing: font-faces are used rarely, it is unlikely a problem to migrate, so not sure it is worth the effort.

I agree, we don't use it, and this is the only Aphrodite syntax not supported in my module.

---

Merge styles: Yep, this can be done here.

The tricky part with the merge is that it can cause issue with JSS plugin jss-plugin-props-sort because of case like:

merge({
  marginRight: 4
}, {
  margin: 0
});

It should result in { margin: 0 }.

A basic merge implementation will result in { marginRight: 4, margin: 0 }, but it will not work with jss-plugin-props-sort (and is dependant on the JS engine for the order of properties).

I choose to go with a smart merge that result in { margin: 0 } by handling shorthand properties, but it requires a listing of all shorthands (css-shorthand-properties.

Does it look like the right solution to you ? (it impacts the bundle size).

---

Regarding the above changes in JSS repo, feel free to send a PR!

Yes, I'll see with my team to open PRs in the coming months.

---

I just made you admin of this repo and added you as owner of the npm package, so that I don't block you and you can update this package directly.

Awesome thanks, I'll make a PR to refactor it once the compatibilities have been created as JSS plugins.

Basically the following roadmap:

1. PRs on cssinjs/jss to add/update plugins for animation, SSR autoprefixing and pseudo elements.
2. Adapt this module with the right merge of styles and use a preset of plugins to match Aphrodite capabilities.

[kof]

Reg. props-sort, I don't understand what the problem is, its fine if the final version is { marginRight: 4, margin: 0 }, the only thing jss-plugin-props-sort will do is to sort this to { margin: 0, marginRight: 4 } . So basically marginRight extends margin, instead of margin completely overriding marginRight. The reason this plugin exists is because this is a more natural behaviour and used by default in react-native.

If the problem is current compatibility - user code relies on the ability to override all specific margins with margin for example, then I see 2 possible solutions:

1. refactor that user code
2. not use jss-preset-default and set up all plugins manually, skip plugins which are not needed

If first one is not an option, second one seems reasonable. Especially if trying to keep the syntax closer to the aphrodite, you will want to remove some other plugins as well.

I choose to go with a smart merge that result in { margin: 0 } by handling shorthand properties, but it requires a listing of all shorthands (css-shorthand-properties.
Does it look like the right solution to you ? (it impacts the bundle size).

I would not do it, since the first 2 options exist, which don't make you maintain the properties.

[kof]

PRs on cssinjs/jss to add/update plugins for animation, SSR autoprefixing and pseudo elements.

Note that animationName supporting the keyframes object would be a syntax specific to aphrodite-jss and should not be part of the JSS repo, it can be still implemented as a plugin inside of aphrodite-jss.

[SamyPesse]

Reg. props-sort, I don't understand what the problem is

I agree that jss-plugin-props-sort does the right thing (sorting { marginRight: 4, margin: 0 } into { margin: 0, marginRight: 4 })

But this is an issue in the merging (before the plugin enters in consideration):

const styles = StyleSheet.create({
  button: {
    marginRight: 4
  },
  buttonNoMargin: {
    margin: 0
  }
})

function Button(props: { withMargin: boolean }) {
  return <button className={css(styles.button, !props.withMargin && styles.buttonNoMargin)}>{props.children}</button>
}

it should compiles to

.button {
  margin-right: 4;
}

.button-buttonNoMargin {
  margin: 0;
}

With the current implementation of aphrodite-jss, it compiles to:

.button {
  margin-right: 4;
}

.button-buttonNoMargin {
  margin: 0;
  margin-right: 4;
}

Solution 1: don't use jss-plugin-props-sort

it will result (which is good):

.button-buttonNoMargin {
  margin-right: 4;
  margin: 0;
}

But It depends too much on the sorting done on object properties by the JS engine (which I think is not reliable).

Aphrodite does the sorting (in StyleSheet.create) before the merge (which occurs when calling css()). The issue with aphrodite-jss is that the sorting is done after the merge.

Solution 2: use jss-plugin-props-sort but with a smarter merge

A smart merge of { marginRight: 4 } with { margin: 0 } understands that marginRight is a sub-property of margin and that the browser will ignore margin-right.

The merge will result in { margin: 0 }.

Solution 3: refactor that user code

I think it should be handled by aphrodite-jss since the goal of this module is to offer an easy migration path that do not require refactoring all the styles in one step.

---

I still think the best solution is Solution 2: a smarter merge in aphrodite-jss and no changes to jss.

It requires maintaining the listing of properties, but css-shorthand-properties looks well maintained and have a good number of downloads.

Properties order in an object is not reliable in my opinion, see for example this issue about difference between server and browser: Khan/aphrodite#199 .
Aphrodite "solved" the issue by supporting Map instances, but it's not very user friendly to write styles using Map.

I'll keep thinking about this in the coming weeks as we migrate more to JSS, and I'll post my feedback.

[kof]

I get the problem now. I think the simplest would be to remove the sorting plugin and set the plugins up without preset. This way we keep the syntax to aphrodite's more closely and reduce the bundle size at the same time. But if you really want to do it with the smart marge - feel free. I consider this plugin more as a way to migrate.