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

add react-router and split on activity paths #4767

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

gatzjames
Copy link
Contributor

@gatzjames gatzjames commented May 10, 2022

Adds react-router and uses the activity as initial paths to render routes and code-split.

Some learnings:

  • Not having to evaluate all the code in initialisation cuts ~20% of startup time in initial tests I ran from ~5s to 3.5-4s depending on the initial route.
  • We can incrementally add more splitting and routes to cut more unused code from being initialised so this can improve even more.
  • The current paths are based on the activity kept in redux state (home/debug/design/test). We should move these navigations to <Link to="..."/> components and const navigate = useNavigate() hook usage. We might need to untangle some state management that has to do with navigation from redux also e.g. tracking events and other reducers that "listen" on the activity changing.
  • Preloading works but can be done better and automatically once we come with conventions around app routing and data loading in the app
  • Suspense shows an empty div to keep the same loading experience that we have now but it opens up the door to introduce loading/skeleton views for when things take time. Again this should go hand in hand with the data loading abstraction we use.
  • Lazy expects a default export and the
    React.lazy() reactjs/rfcs#64 (comment) seems to be that react will use the module information for optimisations in next versions. The same is true for bundlers (e.g. vite) that use the module info to preload other modules ahead of time and code splitting.

changelog(Improvements): Improved Insomnia's startup time

@marckong
Copy link
Contributor

Awesome work that would benefit us moving forward! I just left a few questions that I would love to learn more about this work!

Copy link
Contributor

@marckong marckong left a comment

Choose a reason for hiding this comment

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

LGTM. I will try to contribute adding unit tests to it. I suppose props in each Wrapper-components can also move else where in the future when we are ready!

Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

Explanation about default exports, and future work would be great but besides that looks like a positive low risk first step.

Comment on lines +113 to +122
const ActivityRouter = () => {
const activity = useSelector(selectActiveActivity);
const navigate = useNavigate();

React.useEffect(() => {
if (activity) {
navigate(activity);
}
}, [activity, navigate]);

return null;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to list or link to the future work to untangle this workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added next steps on the pr details!

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

Tested this locally. LGTM!

Difference is almost not noticeable when doing bootstrap and then app-start, BUT, we do seem to shave off at least 3 to 4 seconds on the packaged app itself from the moment the window opens until the moment we "draw" something. Definitely looks faster.

Really nice work @gatzjames 👍

@gatzjames gatzjames merged commit 34aa27a into Kong:develop Jun 6, 2022
@gatzjames gatzjames deleted the add-router branch June 6, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants