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

Avoid subscribing static components #2166

Merged
merged 5 commits into from Nov 4, 2018

Conversation

Projects
None yet
3 participants
@mxstbr
Member

mxstbr commented Oct 28, 2018

I'm not sure if this breaks anything, but if it doesn't it might prevent some unnecessary re-renders?

Avoid subscribing static components
I'm not sure if this breaks anything, but if it doesn't it might prevent some unnecessary re-renders?
@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 28, 2018

This makes sense in theory. Breaks a ton of tests though...

@mxstbr

This comment has been minimized.

Member

mxstbr commented Oct 29, 2018

What the hell are those errors too... 😱

@kitten

This comment has been minimized.

Member

kitten commented Oct 29, 2018

This looks like this.styleSheet = styleSheet; will need to move up before your call to renderInner, no?

@mxstbr

This comment has been minimized.

Member

mxstbr commented Oct 29, 2018

@kitten why exactly do we do this.styleSheet = styleSheet over and over and over every render?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 29, 2018

I think it can change in SSR scenarios from first construction to final emit?

@probablyup

Makes sense to me and tests seem happy 👍

@probablyup probablyup merged commit 2975ff9 into master Nov 4, 2018

3 checks passed

bundlesize ./dist/styled-components.min.js: 15.05KB < maxSize 16KB (gzip)(8B larger than master, careful!)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the avoid-theme-subscriptions branch Nov 4, 2018

@mxstbr

This comment has been minimized.

Member

mxstbr commented Nov 4, 2018

Should probably also do on native btw?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Nov 4, 2018

We don't have a concept of "staticness" in native at the moment. I looked 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment