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

fix: TypeScript performance of DismissibleBanner.tsx #11075

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Oct 9, 2024

Description 📝

  • Cleans up DismissibleBanner.tsx's TypeScript types in hopes to improve tsc performance 🧼
  • This isn't very scientific, but I believe it is helping 🧪

Preview 📷

Running tsc got 14 seconds faster

Screenshot 2024-10-09 at 12 20 59 PM

Profile shows that DismissibleBanner.tsx type checks much faster

Before After
Screenshot 2024-10-09 at 12 18 25 PM Screenshot 2024-10-09 at 12 40 13 PM
takes 16 seconds to typecheck takes 10ms

How to test 🧪

  • Verify type checking passes
  • Verify there are no regressions with the component, it's types, and it's behavior
  • Profile this if you want to (yarn tsc --generateTrace ./trace --incremental false and inspect with chrome://tracing)

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai added the Performance Increase performance of CM label Oct 9, 2024
@bnussman-akamai bnussman-akamai self-assigned this Oct 9, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner October 9, 2024 16:43
@bnussman-akamai bnussman-akamai requested review from hkhalil-akamai and jaalah-akamai and removed request for a team October 9, 2024 16:43
Comment on lines -37 to -40
/**
* Additional styles to apply to the root element
*/
sx?: SxProps;
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need to specify these props because they exist on the interface that this extends (NoticeProps in this case)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also another random thought, but I wonder if sx could be slow. I assume it is a very heavy type. Wondering if SxProps<Theme> is any better or worse

}

interface DismissibleBannerProps
extends Omit<Partial<NoticeProps>, 'children'>,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if Partial<NoticeProps> might be of interest.

BarPercent.tsx has something similar and it takes 17 seconds to type-check

})<Partial<BarPercentProps>>(({ theme, ...props }) => ({

Copy link
Contributor

Choose a reason for hiding this comment

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

most likely. those used to be types unions I believe, and I remember working on those and may have messed that up trying to actually improve the performance doing an interface inheritance. I will certainly profile more often

@abailly-akamai
Copy link
Contributor

Amazing 🥇

Copy link

github-actions bot commented Oct 9, 2024

Coverage Report:
Base Coverage: 86.96%
Current Coverage: 86.96%

@@ -1,31 +1,20 @@
import Close from '@mui/icons-material/Close';
import Grid from '@mui/material/Unstable_Grid2';
import { SxProps } from '@mui/system';
Copy link
Member Author

Choose a reason for hiding this comment

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

Starting to wonder if importing from @mui/system rather than @mui/material is a main source of the slowness.

Maybe I should hurry up with pnpm. I'm 99.9% will prevent @mui/system imports by nature...

@bnussman-akamai bnussman-akamai merged commit c359ad9 into linode:develop Oct 9, 2024
18 of 20 checks passed
Copy link

cypress bot commented Oct 9, 2024

Cloud Manager E2E    Run #6650

Run Properties:  status check passed Passed #6650  •  git commit c359ad9afc: fix: TypeScript performance of `DismissibleBanner.tsx` (#11075)
Project Cloud Manager E2E
Run status status check passed Passed #6650
Run duration 27m 25s
Commit git commit c359ad9afc: fix: TypeScript performance of `DismissibleBanner.tsx` (#11075)
Committer Banks Nussman
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 430

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

Successfully merging this pull request may close these issues.

4 participants