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

More responsive useLocalStorage hook #1703

Merged
merged 7 commits into from
Mar 21, 2024
Merged

Conversation

vcua-mobify
Copy link
Contributor

In order to trigger react re-renders when the contents of local store are modified, our commerce-sdk-react hooks utilize an in internal hook useLocalStorage. However, our current implementation can get into situations where it does not readily pick up changes in local store and instead holds onto previous state. When this occurs, components that call the hook fail to re-render when they should.

This PR changes the implementation of useLocalStorage to be more responsive to changes in local storage.

Testing:
Do the following steps (you can compare these changes on an instance of the PWA with the old implementation of useLocalStorage)

  • Start the app and navigate to a PDP
  • (Optional) Log in
  • Clear local storage

Before the changes:

  • Observe that nothing happens. If you logged in, you can see that the header still shows you are logged in.

After the changes:

  • UI elements are updated to reflect that things in local store are no longer present such as the add to cart button being disabled, and the header updating to show you are now logged out.

@vcua-mobify vcua-mobify marked this pull request as ready for review March 20, 2024 22:17
@vcua-mobify vcua-mobify requested a review from a team as a code owner March 20, 2024 22:17
@vcua-mobify
Copy link
Contributor Author

This change helps address a UI issue seen in #1696 where properties are being updated in local storage but the header component is not updating to reflect new values (ie. on a hybrid site, log in via SFRA then navigate to PWA - the header should show you are logged in).

kevinxh
kevinxh previously approved these changes Mar 20, 2024
@@ -22,22 +22,25 @@ function useLocalStorage(key: string): Value {
return window.localStorage.getItem(key)
}

const [storedValue, setStoredValue] = useState<Value>(readValue)
const useLocalStorageSubscribe = (callback: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider renaming this function to be something else. Right now it's a function masquerading as a hook, at least with the use prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another observation is that you are defining these functions everytime this hook re-renders.. you might want to look into using the useCallback hook to cache the function implementations between renders to ensure we are being as performant as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively you can define those functions in a separate file, or outside of the hook implementation.

@vcua-mobify vcua-mobify merged commit 1fc73eb into develop Mar 21, 2024
28 checks passed
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

3 participants