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

[V3][Hooks Integration 🪝] Replace last of the getProps #1149

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Apr 24, 2023

Description

There were a few places left in the retail-react-template where we were still using getProps. A) in the pge-not-found page and B) in the _app component. In this PR I'm replacing those uses of getProps with their non-getProps react-query equivalent.

NOTE: Because we use ssrPrepass to get hooks working server-side there was an issue where during prepass the messages aren't available, leading to warnings in the console for missing translations. To work around this I updated the custom error handler to only warn if there are messages defined/loaded and the individual message isn't found.

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

  • Replace _app getProps with useQuery implementation.
  • Replace page-not-found getProps with useServerContext
  • Remove use of the withLegacyGetProps HOC
  • Remove any unused tests.
  • Fix bug in SDK where it was required that you have an extraGetPropsArgs implementation even when you aren't using getProps.
  • Remove getProps HOC from test render function.

How to Test-Drive This PR

  • Load dev server and set basic features of localization, changing the local, seeing the text change on various pages etc.
  • Goto a url where there is no route defined to see that you get a 404 response when the page not found page renders.

Checklists

General

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

@bendvc bendvc requested a review from a team as a code owner April 24, 2023 21:31
@bendvc bendvc requested review from kevinxh and vmarta April 24, 2023 21:31
@salesforce-cla
Copy link

Thanks for the contribution! It looks like @vmarta @kevinxh is an internal user so signing the CLA is not required. However, we need to confirm this.

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @vcua-mobify @bendvc @yunakim714 to sign the Salesforce Inc. Contributor License Agreement.

@bendvc bendvc changed the base branch from develop to v3 April 24, 2023 21:53

const hocs = AppConfig.getHOCsInUse()
const getPropsEnabled = hocs.indexOf(withLegacyGetProps) >= 0

const extraArgs = getPropsEnabled ? AppConfig.extraGetPropsArgs(locals) : {}
Copy link
Collaborator

@kevinxh kevinxh Apr 25, 2023

Choose a reason for hiding this comment

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

what happens if getPropsEnabled is true but extraGetPropsArgs is not defined? Is there a default noop implementation for AppConfig.extraGetPropsArgs ? If not, maybe we should guard undefined extraGetPropsArgs?

@bendvc bendvc merged commit a73c207 into v3 Apr 25, 2023
@bendvc bendvc deleted the remove-all-get-props branch April 25, 2023 21:08
bfeister added a commit that referenced this pull request Apr 28, 2023
* v3:
  split ssr build on local (#1155) (#1156)
  [V3] Update Page Designer (#1128)
  [V3][Hooks Integration 🪝] Replace last of the `getProps` (#1149)
  Fix Page Designer ImageWithText Link component (#1092)

# Conflicts:
#	packages/template-retail-react-app/app/components/_app-config/index.jsx
bfeister added a commit that referenced this pull request Apr 28, 2023
…xtensibility-algo-refactor

* feature/template-extensibility:
  split ssr build on local (#1155) (#1156)
  [V3] Update Page Designer (#1128)
  bring back deleted package lockfiles
  fix bad merge conflict
  [V3][Hooks Integration 🪝] Replace last of the `getProps` (#1149)
  Fix Page Designer ImageWithText Link component (#1092)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-mrt-reference-app/package-lock.json
#	packages/template-retail-react-app/app/components/_app-config/index.jsx
#	packages/template-retail-react-app/app/components/_app/index.jsx
#	packages/template-retail-react-app/app/page-designer/core/component/index.jsx
#	packages/template-retail-react-app/app/page-designer/core/component/index.test.js
#	packages/template-retail-react-app/app/page-designer/core/page/index.jsx
#	packages/template-retail-react-app/app/page-designer/core/page/index.test.js
#	packages/template-retail-react-app/app/page-designer/core/region/index.jsx
#	packages/template-retail-react-app/app/page-designer/core/region/index.test.js
#	packages/template-retail-react-app/app/page-designer/layouts/carousel/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid1r1c/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r1c/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r2c/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r3c/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid3r1c/index.jsx
#	packages/template-retail-react-app/app/page-designer/layouts/mobileGrid3r2c/index.jsx
#	packages/template-retail-react-app/app/pages/page-not-found/index.jsx
#	packages/template-retail-react-app/app/utils/test-utils.js
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
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.

3 participants