Improve integration of Custom Views in tabular pages #3275
Conversation
🦋 Changeset detectedLatest commit: 114760f The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 114760f. |
| const [currentCustomViewLocatorCode, setCurrentCustomViewLocatorCode] = | ||
| useState<string>(); | ||
|
|
||
| useEffect(() => { | ||
| const customViewLocator = Object.entries(customViewLocatorCodes).find( | ||
| ([, locator]) => { | ||
| return matchPath(location.pathname, { | ||
| // strip the search, otherwise the path won't match | ||
| path: pathWithoutSearch(locator), | ||
| exact: false, | ||
| strict: false, | ||
| }); | ||
| } | ||
| ); | ||
| setCurrentCustomViewLocatorCode(customViewLocator?.[0]); | ||
| }, [customViewLocatorCodes, location]); |
There was a problem hiding this comment.
Are the state and the effect needed here? 🤔
| const [currentCustomViewLocatorCode, setCurrentCustomViewLocatorCode] = | |
| useState<string>(); | |
| useEffect(() => { | |
| const customViewLocator = Object.entries(customViewLocatorCodes).find( | |
| ([, locator]) => { | |
| return matchPath(location.pathname, { | |
| // strip the search, otherwise the path won't match | |
| path: pathWithoutSearch(locator), | |
| exact: false, | |
| strict: false, | |
| }); | |
| } | |
| ); | |
| setCurrentCustomViewLocatorCode(customViewLocator?.[0]); | |
| }, [customViewLocatorCodes, location]); | |
| const customViewLocator = Object.entries(customViewLocatorCodes).find( | |
| ([, locator]) => { | |
| return matchPath(location.pathname, { | |
| // strip the search, otherwise the path won't match | |
| path: pathWithoutSearch(locator), | |
| exact: false, | |
| strict: false, | |
| }); | |
| } | |
| ); |
There was a problem hiding this comment.
Thanks for the suggestion. I agree that works as well.
My reasoning for using the useEffect is to reduce the number of matchings needed. Using it, we would be calculating the actual locator only if the location changes (given the mapping object is the same between renders). On the other hand, if we don't use the effect, every time the consumer of this hooks is re/rendered, we will trigger again the calculation.
Maybe this is a micro-optimization, but it also looked natural to me using the useEffect hook.
Anyway, if you prefer not using it, I can get rid of it.
There was a problem hiding this comment.
Using useEffect, useState etc is always a bit tricky as you need to be careful of potential re-renders. In particular with useEffect there is always a risk of ending up in an infinite loop.
If it's not absolutely necessary, I would keep it simple.
As for the "optimizations", matching a path or multiple ones is really a "cheap" operation (you can see the implementation of matchPath, it's just simple JS).
However, if you really do want to optimize the calculation I would suggest to use useMemo as it's more suited for this use case.
There was a problem hiding this comment.
I don't see the potential of an infinite loop here and I'm used to avoiding lists iterations whenever I can, but I agree here it's a non-worthy optimization as the list won't ever be too large.
Refactored here: c94cb17
… not use useeffect
Summary
Improve integration of Custom Views in tabular pages
Description
The initial implementation for integrating the upcoming Custom Views feature in tabular pages components only allowed to provide a single locator for the page, where this kind are supposed to have different exclusive sections (which also have different URLs) so it does not support specifying a locator for an specific tab and this is something needed.
In this PR we refactor the integration so tab page components consumers can provide a map of the locators based on the actual URL (the same mapping it's used for deciding which tab content to render).