@W-17543649 - [SEO] Before calling getUrlMapping check the React Router Config first#2379
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>
matchingStrategy from configmatchingStrategy from config and check "cache" on CACHE_FIRST
…ceCommerceCloud/pwa-kit into W-17543649-seo-allowlist-routes
|
This is a good time to start adding to the README to start filling out the "configuration" section. In this readme we should also explain that if they choose the "cache first" mode, that they will never be able to control those routes in BM, that is a pretty important limitation that we'll have to communicate with the users. |
| // `matchingStrategy == CACHE_FIRST`: if `location.pathname` matches a predefined route, skip the `getUrlMapping` API call | ||
| // `matchingStrategy == API_FIRST`: always call `getUrlMapping` | ||
| const skipMappingCall = | ||
| matchingStrategy === 'CACHE_FIRST' && isRouteDefined(location.pathname, routes) |
There was a problem hiding this comment.
We can probably go ahead and create an enum and a type for the matching strategy configuration so we aren't comparing with a static string here.
…ceCommerceCloud/pwa-kit into W-17543649-seo-allowlist-routes
…ceCommerceCloud/pwa-kit into W-17543649-seo-allowlist-routes
Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>
Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>
bendvc
left a comment
There was a problem hiding this comment.
Overall nice work. I've left some comments to make it a little better.
packages/extension-commerce-bm-seo/src/utils/route-match-utils.tsx
Outdated
Show resolved
Hide resolved
| it('should call the callback and push navigation after callback resolves', async () => { | ||
| const callback = jest.fn().mockResolvedValue(undefined) | ||
| render(<TestComponent callback={callback} />) | ||
| const {unmount} = render(<TestComponent callback={callback} />) |
There was a problem hiding this comment.
Was this just a linting thing?
Description
In the current SEO implementation, the
getUrlMappingAPI is called on every navigation. However, we want to offer customers an opt-in functionality to "allowlist" the routes predefined in code.This PR enables just that - if users set their
routingModeto berouter_first, then the extension will check whether the given URL path has been predefined in code. If the path has been defined, then we route the customer there without calling out to the API. IfroutingModeisapi_first, then the extension will call out to the API on every navigation.Types of Changes
Changes
isRouteDefined()method which checks whether the given URL path matches a route predefined in the application's routes config (excluding routes that end in a catch-all, e.g.path='*',path='RefArch*')routingModeto the extension config.routingMode === 'router_first': Check iflocation.pathnamematches a predefined route and skip thegetUrlMappingAPI callroutingMode === 'api_first': Always call the APIseo-hoc.tsx, check the config value against the new enum,RoutingModeIf
routingMode === 'router_first':a. If original path exists, route there <- This is the newly introduced check
b. If not, call
getUrlMappingi. If there is a valid response, navigate there
ii. If no response, show 404
If
routingMode === 'api_first':a. If there is a valid response, navigate there
b. If no response:
i. If original path exists, route there
ii. If none, then show 404
How to Test-Drive This PR
Checking
router_firstbehaviortemplate-typescript-minimalpackage -> In thepackage.json, the config should be set toroutingMode === 'router_first'.category/womens-clothing. The API is not being called because there is a route predefined.Checking
api_firstbehaviortemplate-typescript-minimalpackage -> In thepackage.json, Set the config toroutingMode === 'api_first'.category/mens. The API is being called.To run unit tests
npm run testfrom theextension-commerce-bm-seopackage rootChecklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization