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

Static attrs caching #1219

Merged
merged 4 commits into from Oct 24, 2017
Merged

Conversation

schwers-zz
Copy link

As mentioned in #1216, the static caching machinery introduced a regression; attrs were not considered during the isStatic calculation.

Note however that this assumes attrs will be static at component instantiation. Is dynamically changing the value of component.attrs supported? If so I think we'll need to add a setter and dynamically recompute isStatic

@mxstbr-bot
Copy link

mxstbr-bot commented Oct 9, 2017

Warnings
⚠️ There are library changes, but not tests. That's OK as long as you're refactoring existing code
Messages
📖 ✅ Jest tests passed

Generated by 🚫 dangerJS

Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Thanks, shipping this!

@mxstbr
Copy link
Member

mxstbr commented Oct 24, 2017

Is dynamically changing the value of component.attrs supported?

I don't know why you'd do that, we'll see if people complain I guess!

@mxstbr mxstbr merged commit 7db73e8 into styled-components:master Oct 24, 2017
@kitten
Copy link
Member

kitten commented Oct 25, 2017

@mxstbr wouldn’t that only be possible if the object is kept around and mutated or if the internals are being accessed? 😅

Definitely not supported, but possible I guess

@mxstbr
Copy link
Member

mxstbr commented Oct 25, 2017

Possible for sure, but that's what I mean—let's wait and see if anybody actually complains, then think about fixing it 😅

@vpicone
Copy link

vpicone commented Nov 12, 2017

@mxstbr I think one reason would be to integrate with 3rd party css libraries. For instance, PaperCSS has a popover property. You might want to change that dynamically. There might be a smarter way of doing that, just a thought.

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.

None yet

6 participants