Restructured types to greatly reduce number of types created during c… #1501
Conversation
🦋 Changeset is good to goLatest commit: c336eba We got this. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@JakeGinnivan I must say that I like changes so far, seems cleaner in many places. This still needs some work and testing but looks really great at this point in time. I would like to applaud this effort and preparing such a nice PR 👏 |
|
Thanks @Andarist, I will keep working through all the feedback. I think things can be improved in a few places and I might try to write some tests around the types as well. Likely will be a few days, have a busy weekend this weekend so haven't been able to do much on it. I was also copying types between 3 projects so need to ensure all actually are in sync to test properly. |
JakeGinnivan
force-pushed
the
feature/improve-type-performance
branch
from
9c9d70a
to
c88a1f4
Compare
|
I'm also trying to write down as I go. https://medium.com/@jakeginnivan/improving-emotion-10s-typescript-types-622e2d172f6f This is slowing me down, sorry about that but it's helping me make sure the changes are more deliberate. |
|
@JakeGinnivan Don't worry - it's not like we want to rush this. You are doing a tremendous effort here only because you are motivated to do so on your own - what I'm trying to say, I don't expect anything from you, it's OSS. However, I'm of course grateful for all your efforts 👍 The writeup itself is a great idea and can't wait to dive into it - I'm going to be busy in the following days, but I promise to look at it and on your latest commits afterwards. |
|
@hsz not really, this is somewhat a breaking change and as we are preparing v11 right now it’s just better to include this there. We are going to release a prerelease version with those types and without any breaking changes (other than that). So you should be able to switch to beta v11 line easily if you want to use those typings. |
JakeGinnivan
force-pushed
the
feature/improve-type-performance
branch
from
eee5398
to
c336eba
Compare
Codecov Report
|
|
Sorry for the (somewhat) off topic: is there a list of goals or an umbrella issue for v11 anywhere? A plan or timeline? All I was able to find was the v11 milestone tagged issues. |
|
@elektronik2k5 there is no such umbrella ticket at the moment, but it will be probably created soon @JakeGinnivan thanks for working on this so hard, I'm going to merge this into v11 right now and comment here in some time, once this gets published to npm (under |
|
This has just been published as 11.0.0-next.1 |
|
Hi! I'm not totally sure if it was addressed somewhere before, but when I update to the I've isolated the usecase in this codesandbox -> https://codesandbox.io/s/charming-nobel-7nbh9
This seams to be related to this PR, so I would like to know if it's an issue (and I will open a real issue) or if I'm missing something obvious 😅 Thanks for this great library! Really looking forward the v11 😍 |
|
As you have used |
|
Yeah, this is a bug for sure. Will fix and add some tests. |
|
Thanks for the repro @fabien0102 , we don't use that particular overload in our codebase so I missed it. PR up. |
This patch essentially backports emotion-js/emotion#1501 to dramatticaly reduce the number of types generated during compilation. This reduces the number of types from ~700k to 200k which translates to more than 50% reduction in type check time and memory consumption. Using the command `tsc --noEmit --extendedDiagnostics` | Metric | Before | After | |:------|--------:|------:| | Files | 1189 | 1189 | | Lines | 484378 | 484571 | | Nodes | 892394 | 893030 | | Identifiers | 296394 | 296638 | | Symbols | 769880 | 719992 | | Types | **705230** | **202553** | | Memory used | **1024710K** | **747802K** | | Assignability cache size | 576214 | 382608 | | Identity cache size | 7697 | 6167 | | Subtype cache size | 18326 | 18307 | | I/O Read time | 0.36s | 0.30s | | Parse time | 2.13s | 1.76s | | Program time | 4.28s | 3.48s | | Bind time | 1.25s | 0.94s | | Check time | **40.85s** | **16.66s** | | Total time | **46.38s** | **21.09s** |
* build(tsc): Refactor emotion types so we use less memory This patch essentially backports emotion-js/emotion#1501 to dramatticaly reduce the number of types generated during compilation. This reduces the number of types from ~700k to 200k which translates to more than 50% reduction in type check time and memory consumption. Using the command `tsc --noEmit --extendedDiagnostics` | Metric | Before | After | |:------|--------:|------:| | Files | 1189 | 1189 | | Lines | 484378 | 484571 | | Nodes | 892394 | 893030 | | Identifiers | 296394 | 296638 | | Symbols | 769880 | 719992 | | Types | **705230** | **202553** | | Memory used | **1024710K** | **747802K** | | Assignability cache size | 576214 | 382608 | | Identity cache size | 7697 | 6167 | | Subtype cache size | 18326 | 18307 | | I/O Read time | 0.36s | 0.30s | | Parse time | 2.13s | 1.76s | | Program time | 4.28s | 3.48s | | Bind time | 1.25s | 0.94s | | Check time | **40.85s** | **16.66s** | | Total time | **46.38s** | **21.09s** |
|
How to use |
|
You don't have to use CreateStyled to provide Theme type, you can just augment the Theme interface as described here: https://deploy-preview-1600--emotion.netlify.com/docs/typescript#define-a-theme |
|
Aha! That's pretty cool. But that only works for me when wrapping html tags directy. When I'm wrapping an existing component, the theme is
Should I open a new ticket? Great work with the library by the way! |
|
Yes, you can open a new ticket. Please include a repro case in it. |
…ompilation (emotion-js#1501) * Restructured types to greatly reduce number of types created during compilation BREAKING CHANGE: There are a few breaking changes, see below * withTheme now infers types properly and may require removing the manually specified generic parameter * The Theme generic parameter has been removed from a number of types exported from `@emotion/styled` and `@emotion/styled-base`. * Introduced new CreateThemedStyled type which is exported from emotion-theming, use type to create your own themed `styled` export. See updated docs * WithTheme should be imported from emotion-theming * CreateStyledComponentExtrinsic, CreateStyledComponentIntrinsic and CreateStyledComponentBase all have been replaced with CreateStyledComponent * Fixing tests * Fixed a bunch of tests and improved TypeScript docs * Updated a bunch of the TypeScript docs * Removed WithTheme type, not sure it's usage and there is no tests * Few small cleanups around styled-base * Fixed tests in a few more packages * Fixed serialise tests after changes in emotion-js#1236 * Removed failing redundant test in sheet typescript tests. It is failing with the same compilation error as the previous line, but formatting is causing the test failure * Removed line with expected error, I am not sure the reason it should be failing. * Need to bump the typescript version for styled-base for the union type test * TypeScript tests passing * Upgrade build image version to get newer version of yarn * fix: styled component with static API * Added changes in https://github.com/JakeGinnivan/emotion/pull/1/files to other functions with similar signatures * fix: type issue where styled component passed in * Add some additional tests around theming and fix them * Restrict css function to css interpolation * Fixed emotion-theming linting issue * Reversed some incorrect type changes, withComponent has to include the previous components props otherwise styles on the original component may fail at runtime * Fixed some accidently formatted package.json files * Allowed theming of CreateStyled and StyledTags * Restructured generic type params to make it explicit about what component props should transfer with withComponent and which shouldn't Added tests * Cleaned up some tests and added additional assertions * Default the type of SpecificComponentProps in StyledComponent * Reverted changes around ThemeProvider and added tests * Added changeset * Fixed ThemeProvider after revert * Update tslint rules to fix error * Fixed linting issues * Fixed import path for css and clarified docs * Added comment about fragment shorthand without babel being a typescript limitation * Removed breaking change around some of the internal types It's implementation detail user doesn't need to know * Reverted changes around removing function interpolation from the return types of function interpolation * Renamed Omit to DistributiveOmit To make it clear it's different to the inbuilt Omit * Removed duplicate intersected type * Renamed all usages of SFC to FC * Fixed poor grammar * Ignore lint rule rather than exporting type * Updated generic constraints * Reverted TypeScript version bump in create-emotion types * Sync docs and test code * Constrained Theme to extend {} * Add tests for broken examples in emotion-js#1298 * Fix typo * Add test which verifies emotion-js#1226 is fixed by type changes
…ompilation
BREAKING CHANGE: There are a few breaking changes, see below
@emotion/styledand@emotion/styled-base.styledexport. See updated docsFixes #1496
What:
This PR simplifies a number of types to try and speed up compilation of TypeScript projects when using emotion
Why:
Our project over doubled compilation time when we upgraded from emotion 9 to emotion 10. This brings compilation time back down to emotion 9 times.
How:
I reduced complexity in the types by removing mapped typeds.
Checklist:
Want to get initial feedback, I have consumed all these types in our large project and have gone through a few iterations of fixing issues already.