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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

added failing test #3126

Closed
wants to merge 1 commit into from
Closed

added failing test #3126

wants to merge 1 commit into from

Conversation

TomPridham
Copy link
Contributor

  • Links to documentation:
  • Link to GitHub or NPM:
  • Type of contribution: new definition | addition | fix | refactor

Other notes:
this is related to: #2933
the attrs definition doesn't work as is. i think the problem lies here: https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/styled-components_v4.x.x/flow_v0.75.x-/styled-components_v4.x.x.js#L95
im not sure how to go about fixing it as i'm not sure what exactly is happening there.
tagging @omninonsense since you worked on this last 馃檹

@omninonsense
Copy link
Contributor

Hi @TomPridham!

When I was writing the libdef, Flow couldn't express what styled-components components are.

Here are the current trade offs (for Flow up to 0.89):

  1. The issue you highlighted here is explained in Add type-defs for styled-components v4聽#2933 (comment). However, since Flow 0.89 we can be a lot more expressive and safe, but I didn't have time (yet) to port the typedefs (see Add type-defs for styled-components v4聽#2933 (comment))
  2. No support for tagged template literals, so I didn't bother typing the interpolations. I went into more detail in Add type-defs for styled-components v4聽#2933 (comment). However, @goodmind pointed out in Add type-defs for styled-components v4聽#2933 (comment) this was fixed in v0.92

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.

I'd rather not add just a failing test, as that messes up future PRs that don't address this issue. Please add a fix as well or close.

@TomPridham
Copy link
Contributor Author

sorry, i opened this to get insight on a way to fix the test. i haven't had time to look at it yet. i'll look at it this weekend to see if i can come up with something to fix it

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

3 participants