-
Notifications
You must be signed in to change notification settings - Fork 16
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
SPA Lazy Imports #512
SPA Lazy Imports #512
Conversation
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.
It looks great, and it's much faster! 🚀
Just had questions about the testing
@ekraffmiller I added a component test for the AppLoader component and solved merge conflicts 👍🏼 |
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 good, approved!
Ran tests in local, no issues discovered. This will be tested more over time. |
What this PR does / why we need it:
Applies react lazy imports for page modules. Each page within the SPA application build will be a separate module with its corresponding javascript and css. This will reduce the initial load time, loading only the assets needed for the SPA requesting path.
Which issue(s) this PR closes:
Special notes for your reviewer:
I made changes to the i18n configuration.
Several locale files were deleted and their content was moved to a shared file --
shared.json
.Suggestions on how to test this:
Code visual inspection and re-run of tests actions.
This is difficult to test, as it is a progressive performance enhancement, that's why I suggest re running github action tests.
Another option is on your local environment, run
npm run build
at the root folder of the project in this particular branch to see the build result and maybe do the same indevelop
branch and compare them.Does this PR introduce a user interface change? If mockups are available, please link/include them here: No.
Is there a release notes update needed for this change?: No.
Additional documentation: