-
Notifications
You must be signed in to change notification settings - Fork 851
Add TS check to build process #24329
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
Conversation
|
Caution: This PR has changes that must be merged to WordPress.com |
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
Boost plugin:
|
anomiex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build changes seem ok, but we'll have to wait until the build is actually passing to get any idea of the effect of this change on the build time.
…' as variant to WPButton" This reverts commit 36b7295.
@anomiex may be have a look at this now since the build is all green? |
| * The formatting style for legend item display. If not provided, it defaults to showing legend label after count | ||
| */ | ||
| showLegendLabelBeforeCount: boolean; | ||
| showLegendLabelBeforeCount?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More info here #24474 (comment)
anomiex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it adds ~8 seconds of work for js-packages/components (plus a bunch more for the composer update, sigh), ~8 seconds of work for plugins/boost, and ~65 for plugins/jetpack. The last is most unfortunate, since the high degree of parallelism already in Jetpack's build probably means the CPUs are already maxed out.
As for actual timing, it's hard to tell. The latest run on this PR took 9 minutes for the build step versus 8 for master on the parent branch (fortunately this PR built everything so those are directly comparable). Other builds of the branch have ranged from 8:30 to 11. But due to whatever random factors are involved I see other recent master builds have ranged even higher. So 🤷
|
Great news! One last step: head over to your WordPress.com diff, D80527-code, and deploy it. Thank you! |
|
r246416-wpcom |
After the introduction of TypeScript, we need to ensure that no PR breaks any TS types like #24276 (comment) did.
Changes proposed in this Pull Request:
buildscript tojs-packages/componentsjs-packages/components,plugins/boostandplugins/jetpackfor now.Jetpack product discussion
See Next Steps and the related discussion in comments in #24294
Does this pull request change what data or activity we track or use?
No
Testing instructions:
js-packagesclasNameinstead ofclassNameinprojects/js-packages/components/components/record-meter-bar/index.tsxjetpack build js-packages/components -vprojects/plugins/jetpack/_inc/client/at-a-glance/backup-upgrade/bar-chart.tsxjetpack build plugins/jetpack -vjetpack build --allNote: Initially the build should fail because we have TS errors in master which are being fixed in #24294
So, we need to fix those TS errors and merge into master before this PR can be merged.