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

Add styled-jss to benchmarks #780

Closed
wants to merge 1 commit into from
Closed

Conversation

lttb
Copy link

@lttb lttb commented Jan 18, 2018

Thank you a lot for this project!

And we would be glad to add styled-jss to benchmark list. There are two versions: the core and the default version with some plugins out of the box, so the core is a bit faster.

There are some results on my machine

image

image

image

 

Thanks!

@necolas
Copy link
Owner

necolas commented Jan 18, 2018

react-jss is already included. What's different about styled-jss that makes it worth also testing?

@kof
Copy link
Contributor

kof commented Jan 21, 2018

It has different performance characteristics and api is similar to styled-components. You will be surprised by the updates perf.

@necolas
Copy link
Owner

necolas commented Jan 21, 2018

So which library is best to include here?

@kof
Copy link
Contributor

kof commented Jan 21, 2018

those are 2 separate react integrations using same core, I would include both

@necolas
Copy link
Owner

necolas commented Jan 21, 2018

I don't really want to include multiple variants for each library.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

well, its not a variant, a lot of overhead comes from integration with react, depending on how you do it, you get different results.
They have also very different API's.

I would also include just core jss library, because one can perfectly use it without any integrations and without plugins.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

I hope the selectbox will survive the list of libs haha. Great job by the way on benchmark itself.

@necolas
Copy link
Owner

necolas commented Jan 21, 2018

I can include whichever react integration is fastest.

Multiple integrations running in the same app often end up with problems like this. At the moment, the different core libraries don't even avoid colliding with each other's styles (fela and styletron)

@kof
Copy link
Contributor

kof commented Jan 21, 2018

I think you are not going to have this issue, you are not rendering both on the same screen. They were designed initially to use either of them, so we didn't think of what happens if one uses both on one screen, though, there are valid cases and we should support this behavior. In any case this should not be a problem for the benchmark.

If you get any troubles, I will personally fix it. You can always ask me to update the implementations if you want to update the bench and there is a breaking change or something.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

Regarding fela and styletron, how did that happen? Is there no cleanup operation after switching library?

@necolas
Copy link
Owner

necolas commented Jan 21, 2018

Most libraries don't support being cleaned up, and tbh they should all work together if care is taken to namespace the CSS you create

@kof
Copy link
Contributor

kof commented Jan 21, 2018

In that case mb an iframe is the right thing to do, environment for testing should be clean and isolated, ideally. Doesn't need to be remote document though.

@necolas
Copy link
Owner

necolas commented Jan 21, 2018

This PR doesn't look like it includes vendor prefixing or camel case properties. jss is currently an implicit dependency too.

Using an iframe isn't feasible or important. Most libraries won't work rendering into an iframe from another document. Makes no difference to what is being tested either.

const COLORS = ['#14171A', '#AAB8C2', '#E6ECF0', '#FFAD1F', '#F45D22', '#E0245E'];

const Box = styled(View)({
alignSelf: 'flex-start',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lttb you are using camel case, but no plugins!

@kof
Copy link
Contributor

kof commented Jan 21, 2018

Is there any base features set that every other library is supporting? We could include camel-case for e.g. if it makes the comparison more fair.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

Also yeah, jss import here is implicit, should explicitly add the dependency.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

I just realized what you mean by 2 variants.

@kof
Copy link
Contributor

kof commented Jan 21, 2018

Hmm, yeah 2 variants of styled-jss is a suboptimal thing to have, on one side jss-preset-default adds way more features than other libs have, so it makes it a bit slower on micro level, on the other hand it is what users will use most of the time.

cc @lttb

@kof
Copy link
Contributor

kof commented Jan 21, 2018

I think we should probably just use the default styled-jss implementation, but instead add a pure jss version without plugins or with a minimal set of them.

@necolas
Copy link
Owner

necolas commented Jan 22, 2018

I'll only include one jss example. The performance of styled-jss and react-jss seems about the same for the mounting tests, and react-jss seems to be the more popular way to use jss. styled-jss-core is faster but is missing expected features.

I noticed an issue testing this locally. If you run styled-jss before styled-jss-core, you get slower styled-jss-core results.

image

image

@necolas
Copy link
Owner

necolas commented Jan 22, 2018

Closing this for now. Thanks for the PR

@necolas necolas closed this Jan 22, 2018
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 this pull request may close these issues.

3 participants