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

chore: pin TS 3.3 because of performance of 3.4 #338

Merged
merged 1 commit into from
May 16, 2019
Merged

Conversation

havenchyk
Copy link
Contributor

@havenchyk havenchyk commented May 16, 2019

Description

There is an issue with TS microsoft/TypeScript#30819 that in conjunction with latest updates of @types/react slows down our storybook build.

Long read about how that happened

Results after reverting packages on my machine

Before. After.
2.5 min for manager and 3.17 min for preview 42 s for manager and 56 s for preview

Caveats

yarn build:storybook takes a couple of seconds more with @types/react@16.8.14

Be sure we don't use features that work only in 3.4+ TS - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#typescript-34

Next version 3.5 works better, but still has worse performance than 3.3, upgrade should be considered after the release

How to test

  • run rm -rf node_modules && yarn && yarn storybook and check time

Review

- [ ] Annotate all props in component with documentation
- [ ] Create examples for component

  • Ensure that deployed demo has expected results and good examples

- [ ] Ensure that visuals specs are green See the documentation

Copy link
Collaborator

@denieler denieler left a comment

Choose a reason for hiding this comment

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

Oh, this is sooo bad from Microsoft side...

Thanks for finding this @havenchyk ! It's amazing! 🚀

Edit: irresponsible... this is the word :)

@toptal-devbot
Copy link
Collaborator

Successfully deployed demo at https://picasso.toptal.net/revert-ts-3.3

@MilosMosovsky
Copy link
Contributor

❤️ Oh wow @havenchyk you are killer

@havenchyk havenchyk self-assigned this May 16, 2019
@havenchyk havenchyk force-pushed the revert-ts-3.3 branch 2 times, most recently from 3fe9718 to e9f86ed Compare May 16, 2019 12:27
@havenchyk havenchyk changed the title chore: revert TS and @types/react chore: pin TS 3.3 because of performance of 3.4 May 16, 2019
@toptal-devbot
Copy link
Collaborator

Successfully deployed demo at https://picasso.toptal.net/revert-ts-3.3

@havenchyk
Copy link
Contributor Author

havenchyk commented May 16, 2019

Update:

reverting @types/react doesn't make any significant difference with TS 3.3, latest version of @types/react - >16.8.14 just highlighted the performance problem

Copy link
Contributor

@vedrani vedrani left a comment

Choose a reason for hiding this comment

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

Great finding :)

@@ -128,7 +128,7 @@
"styled-components": "^4.2.0",
"ts-jest": "^24.0.2",
"ts-loader": "^5.4.5",
"typescript": "^3.4.5",
"typescript": "3.3.4000",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set alert to remind us to try it again for 3.5 or 3.4.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'll receive a PR from @depfu, I'll create a jira ticket as a reminder :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@toptal-devbot
Copy link
Collaborator

Successfully deployed demo at https://picasso.toptal.net/revert-ts-3.3

@havenchyk havenchyk merged commit ce52f89 into master May 16, 2019
@havenchyk havenchyk deleted the revert-ts-3.3 branch May 16, 2019 13:12
@toptal-devbot
Copy link
Collaborator

🎉 This PR is included in version 1.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@toptal-devbot
Copy link
Collaborator

Successfully deployed demo at https://picasso.toptal.net/revert-ts-3.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants