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

Update styled-components_v4.x.x.js #3609

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

migueloller
Copy link
Contributor

These changes remove the coercion of props to exact and properly uses the component's instance.

Would love to hear from the type maintainers if this is a welcome change and what I can do to help make the types better!

@pascalduez
Copy link
Member

/cc @omninonsense @LoganBarnett

@goodmind
Copy link
Contributor

Wouldn't this break some tests?

@migueloller
Copy link
Contributor Author

@goodmind, it looks like the second commit broke some tests but it might be worth checking if the tests are checking for the right think. As it is right now, if you have a component that uses attrs and the props object isn't exact, it will be made exact for you, which results in unintended side effects. For example, if you spread props to a component to pass things like children or className.

@LoganBarnett
Copy link
Contributor

@pascalduez I don't recall working on this libdef nor do I have recollection of using this library. I'm happy to help weigh in though! If you need that please direct me to something specific you'd like me to look at :)

@pascalduez
Copy link
Member

@LoganBarnett Oh sorry, I must have been confused with another library then. ;)

@omninonsense
Copy link
Contributor

omninonsense commented Oct 30, 2019

The reason exact is used because of how spread in some inexact circumstances currently behaves (or "misbehaves").

There are some pretty cool and upcoming changes to how spreads will behave in Flow (Search for [spreads] commits in the flow repo).

Once that is released (probably 0.111?), I think think that most of this will need quite a bit of reworking because it will start generating a lot of errors. So, for the time being, I would keep the typo commit, and drop the other two commits. Once the spread changes are live, we can revisit these libdefs.

@migueloller
Copy link
Contributor Author

@omninonsense, that sounds good. I'll follow up once the spread semantics release is out! For now I'll drop the non-typo commits.

@omninonsense
Copy link
Contributor

LGTM! ❤️

@AndrewSouthpaw
Copy link
Contributor

I'm wondering about the build? Was it broken previously for this libdef @omninonsense ?

Copy link
Contributor

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

n/m, obviously the build was broken previously because this change couldn't have caused all those errors. Merging.

@AndrewSouthpaw AndrewSouthpaw merged commit bac16db into flow-typed:master Nov 14, 2019
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