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

Improve performances #479

Closed
wants to merge 2 commits into from

Conversation

@neoziro
Copy link

commented May 5, 2019

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 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 switched to rollup in order to optimize build, provide umd and
    improve tree shaking

What are the breaking changes?

See #470 for all history and original code.

@neoziro neoziro referenced this pull request May 5, 2019
@codecov-io

This comment has been minimized.

Copy link

commented May 5, 2019

Codecov Report

Merging #479 into master will decrease coverage by 1.82%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage     100%   98.17%   -1.83%     
==========================================
  Files           1        1              
  Lines         173      219      +46     
==========================================
+ Hits          173      215      +42     
- Misses          0        4       +4
Impacted Files Coverage Δ
src/index.js 98.17% <97.22%> (-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 af33c08...3d671b3. Read the comment docs.

@neoziro

This comment has been minimized.

Copy link
Author

commented May 8, 2019

@jxnblk are we ready to merge?

@jxnblk
Copy link
Member

left a comment

Sorry for the delay! It's been a busy week 😬 This is looking good! I left some comments, and I'm happy to help out with some of these changes. I don't think you needed to close the previous PR and open a new one, so feel free to keep all the work in this one from now on. I did start a couple branches to separate the rollup config, but they might have conflicts now (see https://github.com/styled-system/styled-system/tree/smooth-perf-only & https://github.com/styled-system/styled-system/tree/smooth-rollup)

If you don't have time to tackle some of these changes, we can merge this into another working branch to continue forward

"sideEffects": false,
"scripts": {
"prepare": "mkdirp dist && npm run build:cjs && npm run build:esm",
"prepare": "rollup -c",

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

This should still be separated into a separate PR before landing

})
})
// Impossible to ensure, due to perf issues
// test('long form prop trumps aliased props', t => {

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

This commented code should be removed

test('parses object values', t => {
const style = width({
width: {
_: '100%',
2: '50%',
0: '100%',

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

If the _ key no longer works, this would mean a breaking change. Is there anyway to support the current functionality and plan for changing this in v5?

This comment has been minimized.

Copy link
@neoziro

neoziro May 12, 2019

Author

In fact, the previous behaviour was not correct for me. An object was like an array:

// previous behaviour
{ _: '100%', 2: '50%' } // ['100%', '50%']
// current behaviour
{ _: '100%', 2: '50%' } // [, , '50%']

For me it is the correct and the expected behaviour, it is more a bug fix than a breaking change.

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

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

The 2 key was used to demonstrate skipping a breakpoint. Can this change be reverted?

This comment has been minimized.

Copy link
@neoziro

neoziro May 12, 2019

Author

See previous comment.

@@ -11,7 +11,7 @@ import {
textStyle,
colorStyle,
borders,
} from '../src'
} from '../src/index'

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

nb: this change is unnecessary

@@ -398,39 +487,39 @@ export const size = mapProps(props => ({

export const verticalAlign = style({ prop: 'verticalAlign' })

// flexbox
// Flexboxes

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member
Suggested change
// Flexboxes
// flexbox
export const flexDirection = style({ prop: 'flexDirection' })
export const flex = style({ prop: 'flex' })
export const justifySelf = style({ prop: 'justifySelf' })
export const alignSelf = style({ prop: 'alignSelf' })
export const order = style({ prop: 'order' })

// grid
// Grid

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member
Suggested change
// Grid
// grid

// variants
// Variants

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member
Suggested change
// Variants
// variants
.npmignore Outdated
CODE_OF_CONDUCT.md
/*
!/dist/**/*.js
!/props.js

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

nb: personally I prefer not using negation for the ignore files

@@ -0,0 +1,688 @@
"use strict";

This comment has been minimized.

Copy link
@jxnblk

jxnblk May 11, 2019

Member

Since this is copypasta dist code from the current version, I don't think this should be checked in

neoziro added some commits Apr 28, 2019

Improve performances
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 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 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.
- Object keys or array keys have now equivalent parsing

@neoziro neoziro force-pushed the neoziro:smooth-system-2 branch from e34dab9 to 3d671b3 May 12, 2019

@neoziro

This comment has been minimized.

Copy link
Author

commented May 12, 2019

Hello @jxnblk, I made the changes and I rebased. One test is not passing, the merge of variant, it was not part of the library initially. Also using your merge method instead of the deepmerge package makes a significant less of perf (according to benchmark).


At start, when I come with this PR, I wanted to migrate Smooth UI System to styled-system. But I think it would not be possible, even if the philosophy is very close, we are not going in the same direction. There is a lot of points where we disagree:

  • Jest VS ava
  • Rollup vs Microbundle
  • Split code in several files (code readability)
  • Use of external packages (why copying deepmerge instead of including it?)
  • References in theme (it costs nothing to support it)
  • Naming of properties (color vs textColor?)
  • Breakpoints array (why not specifying 0?)
  • ...

Also, performance seems to not be something very important for you since a lot of other stuff was implemented and merged before this PR. I try to keep up with rebase but it is difficult (eg. the compose of variant).

I understand your choices, but there are different from mines. So I think we can share the same "theming specification" but I will not use styled-system in Smooth UI or in my projects. I use it professionally, and I have my own roadmap, I can't stick to yours sorry.

Thank you for your time, I hope my contribution helped you!

@jxnblk

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@neoziro I'd really love to figure out how to get this landed, and I'd appreciate some patience here. This is a substantial PR and like any large code change, I expect it to take longer than some of the other smaller PRs that have been merged in. I can chip in to help with parts that you aren't interested in working on, but it sounds like maybe this PR's base should change to a next branch so that this can land as a major version change instead of in v4.

As for the requested changes, you originally proposed a performance refactor, but also chose to include changes to the build and file structure – and are proposing changing the testing library as well. I'm not opposed to these changes, but they weren't discussed beforehand and make this PR harder to evaluate. I need to make sure changes in v4 do not break for the current users of this library, and I hope you can understand where I'm coming from here

@neoziro

This comment has been minimized.

Copy link
Author

commented May 13, 2019

@jxnblk yeah sorry for including these changes, it was not part of perf issue. I think it is going to slow for me, anyway I could continue to submit issues and changes. About the PR, I don’t have time to invest more on it, but the core is here, feel free to iterate on it! Thanks for your time and your patience 🙏

@jxnblk

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Going to close this out since it seems like it's not moving forward. Thanks again for all the work on this and sorry this PR didn't work out. Hopefully we can collaborate in the future, and I'll definitely be taking some inspiration from this for the next version of styled-system

@jxnblk jxnblk closed this May 27, 2019

@neoziro

This comment has been minimized.

Copy link
Author

commented May 27, 2019

@jxnblk no problem. You have the PR for history with the flattening of props in compose. It is the key for keep perf constants with several composes. Always happy to help! Thanks for this great library!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.