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

implement implicit element folding, "as" polymorphic prop #1962

Merged
merged 6 commits into from Sep 1, 2018

Conversation

Projects
None yet
4 participants
@probablyup
Contributor

probablyup commented Aug 31, 2018

No description provided.

probablyup added some commits Aug 31, 2018

fold styled(StyledComponent)
This is basically a reimplementation of the extend API, but
implicitly. Folds attrs, styles, and target up so only one
component is mounted but all the styles are preserved.
implement "as" polymorphic prop
This eliminates the need for the withComponent API by allowing the
underlying component that is mounted to be switched out dynamically
at runtime, with no changes to the attributed styles.
@@ -219,6 +219,8 @@ export default (ComponentStyle: Function) => {
options: Object,
rules: RuleSet
) {
const targetIsSC = isStyledComponent(target)

This comment has been minimized.

@kitten

kitten Aug 31, 2018

Member

nit: Shall we improve the variable name here? e.g. isTargetStyledComp or sth?

This comment has been minimized.

@probablyup

probablyup Aug 31, 2018

Contributor

Yeah makes sense

// fold the underlying StyledComponent attrs up (implicit extend)
// $FlowFixMe
if (targetIsSC && target.attrs) finalAttrs = { ...target.attrs, ...attrs }

This comment has been minimized.

@kitten

kitten Aug 31, 2018

Member

I'm starting to think it'd make sense to abstract attrs into a shareable set of utilities or sth... Maybe a future thing.

nit: Btw, I don't think having the lets here makes sense since a ternary would also do the trick and would imply that it won't change later in this fn

targetIsSC
? // fold the underlying StyledComponent rules up (implicit extend)
// $FlowFixMe
target.componentStyle.rules.concat(rules)

This comment has been minimized.

@kitten

kitten Aug 31, 2018

Member

Should this be moved to the ComponentStyle constructor itself? i.e.

new ComponentStyle(rules, attrs, id, child)

where the child is another ComponentStyle; This would also move the attrs merging, I guess? Not quite sure why we don't move the bulk of buildExecutionContext there anyway.

This comment has been minimized.

@probablyup

probablyup Sep 1, 2018

Contributor

Could go either way. Currently ComponentStyle isn't concerned with folding which I think is probably a good thing to separate concerns?

This comment has been minimized.

@kitten

kitten Sep 1, 2018

Member

Yea, I was thinking of a "combined" refactor of StyledComponent and ComponentStyle, but that can certainly wait 👍

@@ -271,6 +293,7 @@ export default (ComponentStyle: Function) => {
const newOptions = {

This comment has been minimized.

@kitten

kitten Aug 31, 2018

Member

I think we might not need this anymore, i.e. we can avoid options altogether.

  • attrs is called before the StyledComponent is created (tagged template literal call) and never again
  • withConfig is called before as well
  • extend will go
  • withComponent will go

So any "cloning" is gone

This comment has been minimized.

@probablyup

probablyup Sep 1, 2018

Contributor

Let's wait to refactor this out for v5 when withComponent is gone?

@kitten

kitten approved these changes Sep 1, 2018

@probablyup probablyup merged commit 60f8a1c into develop Sep 1, 2018

2 checks passed

bundlesize ./dist/styled-components.min.js: 14.81KB < maxSize 16KB (gzip)(1.44KB smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the as-prop branch Sep 1, 2018

@Anber

This comment has been minimized.

Anber commented Sep 6, 2018

Hi!

In v3 I can optimize heavy components by extracting dynamic styles to a separated styled``.
Do I properly understand, that In v4 all styles of a component will be rerendered even if a stylesheet of one styled`` would be changed?

const StaticStyledLink = styled.a`
… // a lot of styles
`;

const DynamicStyledLink = styled(StaticStyledLink)`
  color: ${props => props.color};
`;

In the current version color doesn't affect styles of StaticStyledLink, but it will in v4.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 6, 2018

In the current version color doesn't affect styles of StaticStyledLink, but it will in v4.

The main change in v4 is the styles of StaticStyledLink will be copied over to DynamicStyledLink and then that color style will be applied last.

@Anber

This comment has been minimized.

Anber commented Sep 6, 2018

@probablyup it will be applied, but also all styles will be reapplied and it can be extremely slow in comparison with current v3 behavior.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 6, 2018

@Anber

This comment has been minimized.

Anber commented Sep 6, 2018

@probablyup what happens in one go? I don't mean an initial render, I speak about dynamic styles. If I later change color props, new implementation will rerender all styles, and not only styles of DynamicStyledLink how it is now. It's ok if you have just a few styles, but if StaticStyledLink has a complex stylesheet it will definitely affect performance.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Sep 6, 2018

No, StaticStyledLink will not be recomputed I'm 99% sure.

@Anber

This comment has been minimized.

Anber commented Sep 6, 2018

@mxstbr but all it styles will be recomputed. After changing just one prop you need to process all styles of DynamicStyledLink and StaticStyledLink, instead of processing just DynamicStyledLink styles.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Sep 6, 2018

No we don't since we know StaticStyledLinks styles are static (ComponentStyle.isStatic) and we never recompute static styles? What am I missing, why do you think that isn't the case?

@Anber

This comment has been minimized.

Anber commented Sep 6, 2018

@mxstbr for DynamicStyledLink you generate a new block of styles which also includes all styles from StaticStyledLinks. Then if color is changed, this block (with all styles from DynamicStyledLink and StaticStyledLinks) will be recomputed and reattach to the style node.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Sep 6, 2018

Yeah, <DynamicStyledLink /> will recompute its styling that's true, but any instance of <StaticStyledLink /> won't recompute its style. That's the same as before, any component that has a props interpolation will recompute its styling when re-rendered? I'm still not sure I understand.

@Anber

This comment has been minimized.

Anber commented Sep 7, 2018

@mxstbr but now it's possible to reduce the number of styles which will be recomputed by splitting component into two (or more) components: one with static styles (and it will be never recomputed after initial render) and one with interpolated styles. v4, on the other hand, will optimize this two components by merging into one.
For example, DynamicStyledLink now is rendered like <a class="StaticStyledLinks-zxcv DynamicStyledLink-asdf …"></a> and only DynamicStyledLink-asdf will be recompute in case of props changing. In v4 it will be rendered as <a class="DynamicStyledLink-asdf"></a> and any props changing will trigger rerender of whole DynamicStyledLink-asdf which also contains styles from StaticStyledLinks-zxcv.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Sep 7, 2018

@Anber now I understand what you're saying, and I'm happy to say you're way overthinking this: it doesn't matter.

Here's what conceptually happened before before:

// This CSS is static and will never be recomputed
const staticLinkStyles = css`
  color: blue;
  background: red;
`

// This CSS is recomputed every render
const dynamicLinkStyles = css`
  color: ${props => props.color};
`;

Here's what conceptually happens after:

// This CSS is static and will never be recomputed
const staticLinkStyles = css`
  color: blue;
  background: red;
`

// This CSS is recomputed every render
const dynamicLinkStyles = css`
  color: blue;
  background: red;
  color: ${props => props.color};
`;

Sure, a couple extra rules are re-parsed on every render but our parser is so fast it basically doesn't matter. If you're parsing anyway, parsing 5 extra lines is ~0 overhead.

@Anber

This comment has been minimized.

Anber commented Sep 7, 2018

@mxstbr if the new parser in v4 is as fast as you say it will be great, but in v3 the trick with component splitting makes sense because render of a complex component with hundred lines of styles takes a noticeable period of time.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Sep 7, 2018

@Anber do you have a benchmark showing the parser to be the bottleneck? I'd be very interested in seeing that. /cc @thysultan

@Anber

This comment has been minimized.

Anber commented Sep 8, 2018

@mxstbr I've made a simple example for three cases: static, dynamic and combo dynamic + static. Despite the fact that dynamic + static 15% faster, in absolute numbers the difference is just 10-15 ms. Maybe there are cases where the difference will be bigger, but yes, usually it doesn't matter.

@styled-components styled-components locked as off topic and limited conversation to collaborators Sep 8, 2018

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