[no-Jira] Reduce initial client bundle size #895
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Bundle SizesCompared against e39bab0
|
canac
force-pushed
the
lazy-components
branch
7 times, most recently
from
a82a526 to
ff66c4b
Compare
There was a problem hiding this comment.
I've reviewed all your commits and files up to the last WIP commit.
I have left some comments, but it looks truly amazing! Dynamic imports could save a lot of load time, plus the other benefits you've added here.
I did find that in the last few comments, mostly Lazy load contacts views there were lots of imports with ../../ when you don't need to add them and call straight from src/ or pages/.
| {contactId && accountListId && ( | ||
| <DynamicContactReferralTab | ||
| accountListId={accountListId} | ||
| contactId={contactId} | ||
| onContactSelected={setContactFocus} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Here is a good place to add a skeleton. Not saying you should do this, but I should in my PR.
There was a problem hiding this comment.
The JS for the tabs load in about 100ms. I don't think the skeleton will be visible long enough to warrant spending the time to make one for the lazy-loaded component. But you can create skeletons that will be visible while waiting for the GraphQL data to load because those can take longer.
| @@ -143,7 +143,7 @@ export const SidePanelsLayout: FC<SidePanelsLayoutProps> = ({ | |||
| transform: rightOpen ? 'none' : 'translate(100%)', | |||
| }} | |||
| > | |||
| {rightPanel} | |||
| {rightOpen && rightPanel} | |||
There was a problem hiding this comment.
Do you also want to add LeftOpen to the leftPanel?
There was a problem hiding this comment.
I thought I did already
There was a problem hiding this comment.
Should this be a .tsx file?
There was a problem hiding this comment.
It's .ts right now because it contains no JSX. Let me know if you think I should change it to .tsx. It probably makes more sense for me to rename src/components/Settings/notifications/StyledComponents.tsx to StyledComponents.ts for consistency.
There was a problem hiding this comment.
This should be named StylesComponents as when we add the Tools functionality, we will add shared components to this file.
There was a problem hiding this comment.
The exported component is technically a regular component, not a styled component, so I feel that it would be misleading to rename this file to StyledComponents.tsx. But we can certainly refactor and combine whatever makes sense when we implement the tools.
src/components/Layouts/Primary/TopBar/Items/SearchMenu/DynamicSearchDialog.tsx
Show resolved
Hide resolved
pages/accountLists/[accountListId]/reports/donations/[[...contactId]].page.tsx
Show resolved
Hide resolved
pages/accountLists/[accountListId]/reports/partnerGivingAnalysis/[[...contactId]].page.tsx
Show resolved
Hide resolved
src/components/Contacts/ContactsContext/ContactsContext.test.tsx
Outdated
Show resolved
Hide resolved
Thanks! It should be even better now that I implemented preloading on hover. Unless they're quick clickers or have an especially slow network, hopefully most of the time our users won't even see the lazy loading spinner. |
There was a problem hiding this comment.
First off, Thank you for wortking on this. This is truly amazing what you've been able to do.
I have reviewed all 463 files. due to the amount of files, I haven't been able to do a deep dive into each file, but more of an overview. With that we need to make sure to do a lot more testing on the PR preview. I will continue testing this week and next.
Going forward, we need to limit the amount of files in a PR, I don't know the number yet, but this was too much and could have been split into multiple PRs.
|
@dr-bizz Thanks for the review. Sorry that it was such a big PR. I should have put commit that adjusts import paths in a separate PR instead of tacking it onto this one. Last week, I went through every changed file and made sure that hovering preloads the right chunk and that the buttons open the right modal. I feel very confident about my direct changes, but we can keep testing to try to find unintentional indirect changes. Did you have specific things in mind that you'd like to make sure gets tested? |
Bundle sizes [mpdx-react]Compared against d35fe11
|
| onClick={() => setAddDonationOpen(true)} | ||
| > | ||
| {t('Add New Donation')} | ||
| </Button> | ||
| )} | ||
| </Box> | ||
|
|
||
| <DynamicModal | ||
| <Modal |
There was a problem hiding this comment.
@dr-bizz The AddDonation component is the heaviest and the more important one to lazy load. And like we discussed earlier, lazy loading Modal here does nothing because it's used directly in so many places that it's already part of the bundle. I also think it's a better UX to lazy load the modal body but not the title, so that the user can at least see the title of the modal while the body loads.
Anyway, this commit reverts the Modal lazy loading you added since that change doesn't do anything, but please push back if you disagree.
There was a problem hiding this comment.
I agree, Lazy loading the body is a good way to go for now.
In another PR, we should make the Dynamic Modal component have a property of title so that when it renders, it shows the user the Modal title while loading the Component. There might be some layout shift.
There was a problem hiding this comment.
In another PR, we should make the Dynamic Modal component have a property of
titleso that when it renders, it shows the user the Modal title while loading the Component. There might be some layout shift.
We can definitely look into that. Although it seems like we'd just be reimplementing Modal at that point. And Modal isn't a very heavy component. Removing it from the initial bundle only shaves off 1-2kb when I tried it. Also we'll create a network request waterfall if we lazy load Modal and its content.
canac
added
the
On Staging
|
@dr-bizz I tested everything again in staging, and it's all working correctly as far as I can tell. |
There was a problem hiding this comment.
@canac I've tested this on staging and it looks awesome! Pages and components are loading super fast. This work will greatly increase productivity for users.
When we roll this live, we need to watch for a number of days, as it's such a large change and things could be reported a few days after going live. With that, it might be best to wait to Monday to merge into production, so you can be around if it breaks.
canac
force-pushed
the
lazy-components
branch
2 times, most recently
from
f739ff0 to
da17fb4
Compare
Description
Main improvements:
Checklist: