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

Optimize Server-side performance #667

Merged
merged 53 commits into from
Aug 4, 2022
Merged

Optimize Server-side performance #667

merged 53 commits into from
Aug 4, 2022

Conversation

adamraya
Copy link
Collaborator

@adamraya adamraya commented Jul 14, 2022

Description

First iteration improving Retail React App Server-side performance.

Problem

Analyzing the Chrome DevTools CPU Profiler results we see today we run a quite expensive method buildPathWithUrlConfig() every time we need to build a URL, which includes running the method on every Link rendered by the App.

Solution

The pattern we use to build URLs doesn't change in the App unless we change the App default configuration values or unless we do a hard refresh (e.g. Choosing a different locale using the locale selector in the Footer).

The proposed solution is to use React.Context to provide the URL template literal with the pattern to build URLs to the entire App.

  • The new createUrlTemplate() function generating the URL template literal is executed only once.
  • The buildPathWithUrlConfig() method is replaced and re-implemented by the createUrlTemplate() method. Removing and unrolling loops with a set of conditionals to reflect the potential URL configurations.

Results

Home Page Sever-side

Screen_Shot_2022-07-13_at_10_49_47_PM

Product List Page Sever-side

Screen_Shot_2022-07-13_at_10_54_34_PM

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • Added <MultiSiteProvider /> to AppConfig to provide the multi-site related values to the entire App: locale, site and buildUrl() function that returns a URL template literal with the pattern to build URLs according to the App configuration defined in default.js file.
  • Numerous multi-site related refactors, including fixing tests and adding new tests to cover all the different App configurations.

How to Test-Drive This PR

Using Chrome DevTools:

  1. Start the Retail React App with Node inspector.
npm run start:inspect --prefix packages/template-retail-react-app/
  1. Navigate to the Home page. Open Chrome DevTools, open the Node inspector and click the Profiler tab.
  2. Click the Record button to record a new CPU Profile, refresh home page and stop the recording.
  3. Analyze the result.

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@adamraya adamraya requested a review from a team as a code owner July 14, 2022 00:45
@adamraya adamraya marked this pull request as draft July 14, 2022 00:45
@adamraya adamraya added the wip Work in process label Jul 14, 2022
@adamraya adamraya marked this pull request as ready for review July 21, 2022 21:33
@adamraya adamraya added ready for review PR is ready to be reviewed and removed wip Work in process labels Jul 21, 2022
Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

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

Let me skim the rest of the changes. I'm not too familiar with other changes that were requested. But I may still give a +1 if things look all right to me.

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

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

Besides one comment about locale in AppConfig, everything else looks good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants