-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor(app-shell): add more code split entry points #2997
Conversation
🦋 Changeset detectedLatest commit: bf8f0eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
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 |
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit bf8f0eb. |
9325e29
to
461f7ac
Compare
a46c280
to
95d5914
Compare
95d5914
to
f21ec6f
Compare
6fed502
to
128af36
Compare
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.
🙌
'@commercetools-frontend/application-shell': patch | ||
'@commercetools-local/playground': patch |
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.
I think our template packages are missing
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.
👏🏽👏🏽
<RestrictedApplication | ||
</SuspendedRoute> | ||
<SuspendedRoute> | ||
<ApplicationShellAuthenticated |
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.
In general I think this is a great idea.
However, I would also share some thoughts about lazy loading component with a very high chance of been rendered as this ApplicationShellAuthenticated
, or project and locale switchers. By moving them out from the initial bundle, we are introducing a delay in the user experience since now they will need to be fetched when React detects it needs them.
Maybe the latency for a backoffice application is not such a high requirement because most users will have a wired internet connection, but just wanted to share some concerns I had in the past when using lazy loading too much.
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.
Thanks for the feedback. Yes some parts might have a slight delay now. The <ApplicationShellAuthenticated>
is only rendered if the user is authenticated so I think we can try code splitting it.
In general we can observe the loading behavior / performance and make further adjustments along the way.
Regarding the switcher components, you're right they probably don't need to be code splitted. Let me revert that part.
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.
👏
Branched off #2995
Extracted from #2821
Main goal is to reduce the size of the main bundle by code splitting more components within the App Shell.
Webpack (Starter template with code splitted routes)
![image](https://user-images.githubusercontent.com/1110551/225287402-a99c318c-a345-487b-aea4-2035f6f5c03f.png)
Vite (Playground with code splitted routes)
![image](https://user-images.githubusercontent.com/1110551/225287570-b8f3b487-95de-46bf-a76d-fefd5a7bd2d8.png)