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

Add TypeScript support for styled-component "as" prop. #1796

Conversation

riley-rangel
Copy link

@riley-rangel riley-rangel commented Mar 11, 2020

What: Add TS support for styled-component "as" prop.

Why: Improves overall DX when using "as" prop.

How: Alongside emotion styled-components living in separate repo.

Checklist:

  • Documentation
  • Tests "N/A"
  • Code complete "N/A"
  • Changeset "N/A"

Other Comments:
Been using this package for over a year now, and I love it. One thing that I noticed it was missing was TS support for the "as" prop. It's such a powerful feature that our team enjoys using in the right situation, but the type omission is a bummer. We've been iterating on various TypeScript definitions for it, and I've had some extra time, so wanted to give this a shot here. If anyone has any suggestions or recommendations, please throw a comment up below!

Relates to:
#1137

@changeset-bot
Copy link

changeset-bot bot commented Mar 11, 2020

💥 No Changeset

Latest commit: e00c57b

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Andarist
Copy link
Member

Please note that we are unlikely to merge this into v10 line as we are already preparing v11. Could you please change the base branch and rebase?

I'm going to look into it then, for now I've pinged @JakeGinnivan who has worked hard to improve our v11 typings so he can take a look at the proposed approach.

@JakeGinnivan
Copy link
Contributor

@riley-rangel do you have a code base you are testing this change on.

Could you run tsd --extendedDiagnostics before and after. I've been trying to remove mapped types because they can cause type explosions and a much slower compilation times on larger projects.

@Andarist
Copy link
Member

@JakeGinnivan Those changes don't use any extra mapped types from what I can see. Do conditional types lead to perf problems as well?

@JakeGinnivan
Copy link
Contributor

I think Extract is a mapped type under the hood. Let me give it a spin

@JakeGinnivan
Copy link
Contributor

Taking a quick look on https://github.com/JakeGinnivan/emotion-types-sandbox

Here are the results (a single basic styled component).

Before:
Nodes: 335688
Identifiers: 113082
Symbols: 189188
Types: 52818
Memory used: 216034K
Assignability cache size: 134323

After:
Nodes: 335798
Identifiers: 113129
Symbols: 193848
Types: 60550
Memory used: 244045K
Assignability cache size: 150830

A 12% increase in memory usage and 14% increase in the number of types.

@JakeGinnivan
Copy link
Contributor

Also just checked in our project

Before:
Symbols: 1217383
Types: 255404
Memory used: 817400K
Check time: 24.37s

After
Symbols: 1418829
Types: 328758
Memory used: 1186022K
Check time: 31.13s

Also, I got a compilation error on

export const SearchInput = styled('input')({ color: colors.designSystem.blueAstral })

<SearchInput
     placeholder="Search Components"
     onChange={i => this.handleInputChange(i)}
/>

src/styleguide/src/common/Nav/nav.tsx:68:35 - error TS7006: Parameter 'i' implicitly has an 'any' type.
68 onChange={i => this.handleInputChange(i)}

Not quite sure what the go is, but it has lost some ability to infer correctly with the change.

@JakeGinnivan
Copy link
Contributor

It's an annoying trade off, the smarter you make your types the more intermediate types which get created during compilation. I'm working with someone on the TypeScript team trying to narrow down the patterns in CSS-in-JS libraries in general which are causing these type explosions so hopefully we can fix it in the compiler itself.

Copy link
Contributor

@JakeGinnivan JakeGinnivan left a comment

Choose a reason for hiding this comment

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

With the perf issues this introduces currently I'm not sure it's worth trying to fix it. We have the .withComponent, but when using as it just is not type safe?

My vote is to not land this for now, I'm just not sure the perf issues are worth the better types at this stage.

@Andarist
Copy link
Member

Other than that - from what I understand this has introduced a regression (@JakeGinnivan's SearchInput case). So this definitely is a no go in the current state.

@riley-rangel
Copy link
Author

Sorry guys, with everything going on, I haven't had time to check back on this. In terms of the regression and the performance hit, I will look into trying to resolve these issues.

@JakeGinnivan Thanks for the hint on how to check performance on this. I wasn't aware of this before, so I will enjoy reading up more on that. I will try to refactor w/o mapped types as well. Still learning about TypeScript, so this will be an awesome opportunity to learn more.

@Andarist Thank you for pointing out this PR to @JakeGinnivan. I will continue to work on this on the side as time permits, but in the meantime, if you'd like to keep the PRs list clean, we can close this. I have plenty of things that can help point me in the right direction on this one for now.

@Andarist
Copy link
Member

@riley-rangel k, I'm going to close this for now, but I'm happy to reopen this whenever you ping me to do so. Please also note that this has 0 chance to hit the master (v10) right now - if you want to continue working on this I would encourage you to rebase your work on the next branch (v11). We hope to wrap v11 soon-ish (so it will naturally be merged into master), but can't give any hard estimates.

@Andarist Andarist closed this Mar 16, 2020
@JakeGinnivan
Copy link
Contributor

@riley-rangel no worries, happy to help.

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.

3 participants