Improve performances #470

gregberge · 2019-04-28T16:54:19Z

Following #444, I bring perf improvements from @smooth-ui/system into styled-system.
The goal is to make this library the best one and the universal one.

What has changed?

I switched to the core of @smooth-ui/system, more performant
especially with multiple compose. Composition is now flat, it means you
can compose 10 times and it will not be slower. We are probably in O(n),
where "n" is the number of props.
I splitted the logic in several files to make it clearer
I switched to rollup in order to optimize build, provide umd and
improve tree shaking

What are the breaking changes?

We now rely on the order of props. ES2015 is here and we can rely on
it, see this tweet for context: https://twitter.com/ianstormtaylor/status/1115680037875752963.
We are more strict in breakpoints, we don't accept a non-existing
breakpoint
We don't export utilities like get, themeGet, etc...
PropTypes are no longer included, they can be generated from meta
We don't return array, just an object with all props merged
Properties that default to pixels unit (like margin, padding) no
longer include it
Breakpoints are no longer magic, the smallest has to be 0 if you want it to be

Benchmarks

I added a benchmark in the project, the result:

$ npm run bench

> styled-system@4.1.1 bench /Users/neoziro/Projects/styled-system
> node benchmarks

Initial run...
actual { padding: 10,
  '@media screen and (min-width: 40em)': { padding: 20 },
  marginTop: 10,
  margin: '20px',
  fontSize: 10 }
v4 [ { fontSize: '10px' },
  [ { margin: '20px' },
    { marginTop: '10px' },
    [ [Object], [Object] ] ] ]
Run suite...
actual x 351,931 ops/sec ±0.66% (88 runs sampled)
v4 x 190,533 ops/sec ±0.45% (90 runs sampled)
Fastest is actual

Following styled-system#444, I bring perf improvements from @smooth-ui/system into styled-system. The goal is to make this library the best one and the universal one. What have changed? - I switched to the core of @smooth-ui/system, more performant especially with multiple compose. Composition is now flat, it means you can compose 10 times and it will not be slower. We are probably in O(n), where "n" is the number of props. - I splitted the logic in several files to make it clearer - I switched to rollup in order to optimize build, provide umd and improve tree shaking What are the breaking changes? - We now rely on the order of props. ES2015 is here and we can rely on it, see this tweet for context: https://twitter.com/ianstormtaylor/status/1115680037875752963. - We are more strict in breakpoints, we don't accept a non-existing breakpoint - We don't export utilities like get, themeGet, etc... - PropTypes are no longer included, they can be generated from meta - We don't return array, just an object with all props merged - Properties that default to pixels unit (like margin, padding) no longer include it

TrySound · 2019-04-28T17:30:48Z

package.json

-  "module": "dist/index.esm.js",
+  "main": "dist/styled-system.cjs.js",
+  "module": "dist/styled-system.es.js",
+  "jsnext:main": "dist/styled-system.es.js",


Drop jsnext:main. It is deprecated for a long time.

TrySound · 2019-04-28T17:31:23Z

package.json

-  "main": "dist/index.cjs.js",
-  "module": "dist/index.esm.js",
+  "main": "dist/styled-system.cjs.js",
+  "module": "dist/styled-system.es.js",


Please keep esm prefix. It's more descriptive. es is often used to distribute es6 syntax.

TrySound · 2019-04-28T17:32:04Z

rollup.config.js

+import babel from 'rollup-plugin-babel'
+import replace from 'rollup-plugin-replace'
+import commonjs from 'rollup-plugin-commonjs'
+import { uglify } from 'rollup-plugin-uglify'


Please use rollup-plugin-terser. Terser is more stable.

TrySound · 2019-04-28T17:36:00Z

rollup.config.js

+  const esConfig = Object.assign({}, baseConfig, {
+    output: {
+      file: `${DIST_DIR}/${buildName}.es.js`,
+      format: 'es',


esm format is an alias

TrySound · 2019-04-28T17:37:50Z

rollup.config.js

+    external: [
+      ...Object.keys(pkg.peerDependencies || {}),
+      ...Object.keys(pkg.dependencies || {}),
+    ],


id => !id.startsWith('.') && !id.startsWith('/') is more universal. It will treat as external @babel/runtime/.... Make sure your config does this.

TrySound · 2019-04-28T17:39:24Z

rollup.config.js

+  })
+
+  const globals = {
+    deepmerge: 'deepmerge',


I would bundle this module for umd

gregberge · 2019-04-28T18:39:32Z

@TrySound thanks! Done!

TrySound

I prefer to reuse parts of config instead of extending it over the place. Visually it becomes a bit simpler
https://github.com/istarkov/rifm/blob/master/rollup.config.js

TrySound · 2019-04-28T18:59:54Z

rollup.config.js

+
+function getConfig() {
+  const name = 'styledSystem'
+  const buildName = 'styled-system'


TrySound · 2019-04-28T19:00:44Z

rollup.config.js

+      babel({
+        runtimeHelpers: true,
+        exclude: 'node_modules/**',
+        configFile: path.join(__dirname, 'babel.config.js'),


Config file should work without option

TrySound · 2019-04-28T19:02:46Z

rollup.config.js

+      format: 'esm',
+    },
+    external: id => !id.startsWith('.') && !id.startsWith('/'),
+    plugins: [...baseConfig.plugins, resolve()],


Why resolve plugin here? Only for index.js?

TrySound · 2019-04-28T19:05:55Z

rollup.config.js

+      format: 'umd',
+      globals,
+      exports: 'named',
+      sourcemap: false,


globals is empty - can be removed, exports: named is autoselected since there is not default export, sourcemap is false by default.

TrySound · 2019-04-28T19:06:21Z

rollup.config.js

+      exports: 'named',
+      sourcemap: false,
+    },
+    external: Object.keys(globals),


All packages are bundled. Not necessary

TrySound · 2019-04-28T19:11:57Z

babel.config.js

+    presets: [['@babel/preset-env', { loose: true, modules: false }]],
+    plugins: [
+      'babel-plugin-annotate-pure-calls',
+      ['@babel/transform-runtime', { useESModules: true }],


One thing is missing here. cjs bundle will get esm imports.

Here I added transform-runtime in place and didin't use it for tests and benchmarks. babel runtime is just an optimisation.
https://github.com/istarkov/rifm/blob/master/rollup.config.js#L19

gregberge · 2019-04-29T05:56:35Z

@jxnblk is it OK for you if I switch the test engine to Jest? I had some difficulties with "ava" (watch mode + console logging is not nice). Also I have some unactivated tests that I could activate if I switch.

Also could you please configure Travis to report the test status of this PR?

gregberge · 2019-04-29T05:59:51Z

@jxnblk and of course, I would like to have your advice on breaking changes.

TrySound · 2019-04-29T09:11:04Z

rollup.config.js

+const getBabelOptions = ({ useESModules }) => ({
+  exclude: '**/node_modules/**',
+  runtimeHelpers: true,
+  presets: [['@babel/preset-env', { loose: true, modules: false }]],


You may remove this line since env plugin has always modules: true with rollup-plugin-babel.

flybayer · 2019-04-29T14:02:12Z

Why have you stopped exporting themeGet? I use it a lot for places where I need to set static styles from the theme that don’t need changed by props.

Sure I can just import the theme object, but that doesn’t work for dynamic themes.

StevenLangbroek · 2019-04-29T14:14:46Z

Yeah same on the themeGet. We rely on it pretty extensively.

jxnblk · 2019-04-29T15:20:12Z

🙌 Thanks for this! I think some of the core performance improvements are looking good, and I'll follow up with a few inline questions to get a better sense of what these changes entail. Specifically, the new compose looks great! I do have a few thoughts and questions, so please bear with me.

I was hoping that some of these performance improvements could be introduced without breaking changes (I think it might still be possible), but I'm also open to using this as a starting point for the next major v5. If possible, we could introduce some of the performance changes in v4, but use the current state of this PR as a potential end-state for v5.

Notes

We don't return array, just an object with all props merged

This was an experimental change from v3 to v4. In some benchmarks it looked like letting Emotion handle the merging and flattening of styles on its own was faster than merging them in Styled System when the two libraries were used together. That said, and in light of JSS looking to support Styled System, I think this change definitely should be addressed in v4, and I'm planning on opening a PR for this.

See this issue for more: #471

Properties that default to pixels unit (like margin, padding) no longer include it

This may no longer be the case, but converting numbers to pixels was previously required for Styled Components support. Can you confirm that this works with Styled Components?

Questions

We are more strict in breakpoints, we don't accept a non-existing breakpoint

Can you elaborate on what this means with a code usage example? It looks like this only affects the plain object values?

Breakpoints are no longer magic, the smallest has to be 0 if you want it to be

Not sure I understand. Can you please elaborate? Is this the same as above?

Requested Changes

There are a few changes in here that I'd ask you to decouple from the core performance improvement since they are not really related and can be addressed as separate PRs. I'm more than happy to jump in to help break this up if you don't have much time to work on it.

Keep everything in a single src/index.js file for now. Splitting the code base into multiple modules makes the change look larger than it is and is more difficult to navigate. Seeing a side-by-side of before and after is helpful IMO. If there's some other reason for this change, please explain.
Rollup or other build configuration isn't really related to this change and should be a separate PR. If everything is kept in a single file, I also suspect there isn't much of a need for Rollup. Generally I lean towards something like microbundle for this, especially in smaller libraries.
(Open for discussion) I removed the benchmarks from the core recently. While they are helpful for making changes like this, I think it might make sense to keep in a separate repo (or package in a future monorepo). At the very least, it could be nice to have that on a separate branch/PR.

Should this be a breaking change?

As for whether or not this should be a breaking change, I'm open to discussion and can open a separate issue to track v5. Generally for v5, I was planning on splitting a lot of this up into a monorepo with Yarn workspaces and Lerna. If you think it's worth getting this in as a minor/non-breaking change, I think the following would need to be addressed:

Keep propTypes for now (I think in the next version it should be a separate, optional utility package)
Keep themeGet in core for now (people do use this). This too can be split into a separate package in v5.
I'm not sure if the props ordering is actually a breaking change or not...
Keep breakpoints behavior intact (I'm not entirely sure what changes you made here yet)

If merging this as a non-breaking change isn't easy, let's start working towards a v5 release with a good migration plan. And again, thanks for all your work on this so far!

jxnblk · 2019-04-29T15:22:11Z

@jxnblk is it OK for you if I switch the test engine to Jest? I had some difficulties with "ava" (watch mode + console logging is not nice). Also I have some unactivated tests that I could activate if I switch.

I don't see a good reason to switch to Jest here. Ava is nice for simple libraries like this, and I prefer to use it when React and/or DOM APIs are not required. Totally open to changing this separately, but it seems out of scope for this change.

To run Ava in verbose watch mode you can use npm t -- -wv

Also could you please configure Travis to report the test status of this PR?

I'll look into what's happening here

jxnblk · 2019-04-29T15:35:45Z

It looks like Travis wasn't reconfigured after moving this to the styled-system org. I think it should run tests on the next push, but if not I'll dig into it some more

gregberge · 2019-04-30T11:42:12Z

Hello @jxnblk,

Thanks for your insights! I will answer your points:

I was hoping that some of these performance improvements could be introduced without breaking changes (I think it might still be possible), but I'm also open to using this as a starting point for the next major v5. If possible, we could introduce some of the performance changes in v4, but use the current state of this PR as a potential end-state for v5.

I think it is possible to avoid all breaking changes, except props ordering. I think it is not really a breaking change, so we could release it as v4.x.

Notes

We don't return array, just an object with all props merged

This was an experimental change from v3 to v4. In some benchmarks it looked like letting Emotion handle the merging and flattening of styles on its own was faster than merging them in Styled System when the two libraries were used together. That said, and in light of JSS looking to support Styled System, I think this change definitely should be addressed in v4, and I'm planning on opening a PR for this.

I think it is better to merge styles in styled-system. It is optimized and for me it is not a breaking change.

Properties that default to pixels unit (like margin, padding) no longer include it
This may no longer be the case, but converting numbers to pixels was previously required for Styled Components support. Can you confirm that this works with Styled Components?

I confirm it is working for styled-components, code is cleaner and faster.

We are more strict in breakpoints, we don't accept a non-existing breakpoint

Can you elaborate on what this means with a code usage example? It looks like this only affects the plain object values?

On this specific point, I think we should ignore a non-existing breakpoint, I will adapt the code to just ignore non-defined breakpoint.

Breakpoints are no longer magic, the smallest has to be 0 if you want it to be

Not sure I understand. Can you please elaborate? Is this the same as above?

Today, you don't define the breakpoint 0, is it implicit. I think it should not, to be aligned with other behaviour like the space one. Changing this is a breaking change, because it forces users to modify their theme. I will change the code to be compatible with the current behaviour, but I think we should definitely talk about it for v5.

Keep everything in a single src/index.js file for now. Splitting the code base into multiple modules makes the change look larger than it is and is more difficult to navigate. Seeing a side-by-side of before and after is helpful IMO. If there's some other reason for this change, please explain.
Rollup or other build configuration isn't really related to this change and should be a separate PR. If everything is kept in a single file, I also suspect there isn't much of a need for Rollup. Generally I lean towards something like microbundle for this, especially in smaller libraries.

Yeah we could keep everything in the same file, it is not a problem, I can change the code. About Rollup, I think we should use it, it will result in a better build, and for me it is part from the performance part. So are you OK to keep Rollup (especially because it was reviewed by @TrySound and the config is rock solid!).

(Open for discussion) I removed the benchmarks from the core recently. While they are helpful for making changes like this, I think it might make sense to keep in a separate repo (or package in a future monorepo). At the very least, it could be nice to have that on a separate branch/PR.
Should this be a breaking change?

I added them in this PR to show the perf improvements, of course we could remove them from this PR before merging it. It could be interesting to put them in another repo, yes.

Keep propTypes for now (I think in the next version it should be a separate, optional utility package)

I will let the meta in this version, and also add propTypes, so we will have the best of the both world and no breaking change.

Keep themeGet in core for now (people do use this). This too can be split into a separate package in v5.

Yeah, I will all documented utilities to avoid breaking change.

I'm not sure if the props ordering is actually a breaking change or not...

Strictly, it is but I think users don't define bg and backgroundColor on the same component, it makes no sense. It will be a breaking change, but I think we could consider that it is not. Are you OK with that?

Keep breakpoints behavior intact (I'm not entirely sure what changes you made here yet)
If merging this as a non-breaking change isn't easy, let's start working towards a v5 release with a good migration plan. And again, thanks for all your work on this so far!

As answered before, I will make it work as it work today to avoid breaking change.

About Jest

I will definitely not make the change in that PR, it is not the subject. But I think it is better to switch to Jest instead of ava for several reason:

Jest is widely used (https://www.npmtrends.com/ava-vs-jest)
Jest has a better watch mode
Jest is universal (front-end, back-end, integration...)

So I think it would be a good move (also for contributors) to move to Jest. Anyway it is another subject and it should be discussed in another issue. I will open a new one after this one will be done, one subject at a time.

Conclusion

I will work on the PR and operates these changes:

Remove all breaking changes (except the special one on prop ordering)
Put everything in a single file
I would like to keep Rollup, I think it is better to keep it

Are you OK with these changes?

jxnblk · 2019-04-30T13:47:37Z

@neoziro Awesome! Sounds like we're mostly on the same page :) I'll start a v5 roadmap issue to track some of the changes discussed here, and lets figure out how to get this in as a minor change.

For style object merging, I threw this PR together quickly (could be helpful for you to review) Merge styles into single object #473
I've tried to keep external dependencies out of this project to help encourage adoption. If we're ditching propTypes too, it'd be great if styled-system could have zero hard dependencies IMO
For Rollup, I'm fine introducing it (or microbundle) I'd just rather see it in a separate PR since it's build setup and doesn't need to be coupled to this change. Again, I can chip in to help split up the PR if that's helpful
Same for Jest. I use Jest in a lot of projects that require React or DOM APIs, but it's not really related to the changes proposed here. I do think Ava has a much nicer CLI design than Jest, but that's just like my opinion, man 😎
I'd say leave the benchmarks alone for now (keep them), they can be helpful for this PR. I suspect there might be a better way to test against different versions of the same library, but that's out of scope
I think the implicit breakpoint behavior is something we can discuss for v5 – i.e. it probably makes a ton of sense I just haven't thought about it much yet

I confirm it is working for styled-components, code is cleaner and faster.

This too could be considered a breaking change, but might not affect how most people use this library. Probably need to think on this (don't make any changes yet), but it might be something to shim for a non-breaking change.

Strictly, it is but I think users don't define bg and backgroundColor on the same component, it makes no sense. It will be a breaking change, but I think we could consider that it is not. Are you OK with that?

I think so... It could be helpful to look at a unit test or example that demonstrates what's different – think this is also useful for the changelog

Otherwise, yeah this is looking great! Thanks for working on this!

jxnblk · 2019-04-30T15:17:36Z

Just a heads up that it looks like unitless pixel value support was only recently added to Styled Components, so I think that should be treated as a breaking change styled-components/styled-components#2173

- Put all in a single file: `src/index.js` - Remove all breaking changes

codecov-io · 2019-05-04T13:07:45Z

Codecov Report

Merging #470 into master will decrease coverage by 1.82%.
The diff coverage is 97.24%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
- Coverage     100%   98.17%   -1.83%     
==========================================
  Files           1        1              
  Lines         159      219      +60     
==========================================
+ Hits          159      215      +56     
- Misses          0        4       +4

Impacted Files	Coverage Δ
src/index.js	`98.17% <97.24%> (-1.83%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef126ca...31a3bfa. Read the comment docs.

codecov-io · 2019-05-04T13:07:45Z

Codecov Report

Merging #470 into master will decrease coverage by 1.82%.
The diff coverage is 97.24%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
- Coverage     100%   98.17%   -1.83%     
==========================================
  Files           1        1              
  Lines         159      219      +60     
==========================================
+ Hits          159      215      +56     
- Misses          0        4       +4

Impacted Files	Coverage Δ
src/index.js	`98.17% <97.24%> (-1.83%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef126ca...31a3bfa. Read the comment docs.

gregberge · 2019-05-04T13:08:53Z

I updated the issue, so we have only two breaking changes:

The order of props (https://github.com/styled-system/styled-system/pull/470/files#diff-910eb6f57886ca16c136101fb1699231R65)
The weird parsing of object values as (https://github.com/styled-system/styled-system/pull/470/files#diff-910eb6f57886ca16c136101fb1699231R119)

I think these breaking changes are very small and not "real breaking change" for community. What do you think?

I let the Rollup config in this PR, I don't have time to decouple it.

Thanks!

jxnblk · 2019-05-04T14:39:00Z

Awesome! Looking great! I'll take a closer look at this in a bit.

I think these breaking changes are very small and not "real breaking change" for community. What do you think?

I'm guessing it's fine, but I'll take a closer look at the tests and see if there's anything that looks actually breaking

I let the Rollup config in this PR, I don't have time to decouple it.

No worries, I can split that off into a separate branch later.

I'm not sure what's missing coverage in the tests, but would be nice to keep it at 100%. Don't worry about it for now though – there might be some other changes before merging this in

jxnblk · 2019-05-04T14:46:36Z

test/index.js

-      _: '100%',
-      2: '50%',
+      0: '100%',
+      1: '50%',


The test above was for showing how breakpoints could be "skipped" – why did you change this?

In your previous example it was not skipped, so there was a bug.

https://github.com/styled-system/styled-system/blob/master/test/index.js#L128

It is now fixed, if the key does not point to anything it will be skipped.

jxnblk · 2019-05-04T14:47:47Z

test/index.js

-    backgroundColor: '#111',
-  })
-})
+// Impossible to ensure, due to perf issues


Does this mean that the last prop defined now trumps (i.e. object order)?

Yeah exactly, the last property defined on the object will won. It is very predictable and easy to fix.

jxnblk · 2019-05-04T14:51:12Z

src/index.js

-export const createMediaQuery = n => `@media screen and (min-width: ${px(n)})`
+function getThemeValue(theme, path, initial) {
+  if (!theme) return undefined
+  return callOrReturn(get(initial || theme, path, undefined), { theme })


Is this to support new functionality?

Yeah, it is a new feature (you can release it hidden for now), it permits to have reference values in theme.

const theme = { red: 'red', primary: ({ theme }) => theme.red, }

jxnblk · 2019-05-04T14:54:56Z

src/index.js

  key: 'space',
-  transformValue: getSpace,
-  scale: spaceScale,
+  transformValue: scale(),


Why is this called here?

The transformValue function is now much more permissive than before, so we can even implement the all space logic in it.

scale() is a transform function factory that gives this possibility. It is a factory because of fontSize that has a custom scale.

jxnblk · 2019-05-04T16:14:08Z

Gave you some merge conflicts :) I think this branch should help clear them up https://github.com/styled-system/styled-system/tree/smooth-merge-master

gregberge · 2019-05-05T07:21:41Z

@jxnblk I rebased and opened a new PR #479, I prefer to keep this one as it is for history. In the new one I rebased all in one commit.

gregberge force-pushed the smooth-system branch from 00ed38e to bbcd25e Compare April 28, 2019 16:56

TrySound reviewed Apr 28, 2019

View reviewed changes

Optimize rollup config

32f3e1f

TrySound reviewed Apr 28, 2019

View reviewed changes

gregberge added 2 commits April 28, 2019 23:36

Refactor rollup config

fab8459

Fix tests

fe76d3e

TrySound reviewed Apr 29, 2019

View reviewed changes

gregberge force-pushed the smooth-system branch from a491731 to 0cca58f Compare May 4, 2019 13:07

Refactor code

31a3bfa

- Put all in a single file: `src/index.js` - Remove all breaking changes

gregberge force-pushed the smooth-system branch from 0cca58f to 31a3bfa Compare May 4, 2019 13:07

jxnblk reviewed May 4, 2019

View reviewed changes

gregberge mentioned this pull request May 5, 2019

Improve performances #479

Closed

gregberge closed this May 5, 2019

Improve performances #470

Improve performances #470

Conversation

gregberge commented Apr 28, 2019 • edited

What has changed?

What are the breaking changes?

Benchmarks

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

gregberge commented Apr 28, 2019

TrySound left a 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

TrySound Apr 28, 2019 • edited

Choose a reason for hiding this comment

gregberge commented Apr 29, 2019

gregberge commented Apr 29, 2019

Choose a reason for hiding this comment

flybayer commented Apr 29, 2019

StevenLangbroek commented Apr 29, 2019

jxnblk commented Apr 29, 2019

jxnblk commented Apr 29, 2019

jxnblk commented Apr 29, 2019

gregberge commented Apr 30, 2019

About Jest

Conclusion

jxnblk commented Apr 30, 2019

jxnblk commented Apr 30, 2019

codecov-io commented May 4, 2019 • edited

Codecov Report

codecov-io commented May 4, 2019

Codecov Report

gregberge commented May 4, 2019

jxnblk commented May 4, 2019

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

Choose a reason for hiding this comment

jxnblk commented May 4, 2019

gregberge commented May 5, 2019

gregberge commented Apr 28, 2019 •

edited

TrySound Apr 28, 2019 •

edited

codecov-io commented May 4, 2019 •

edited