-
Notifications
You must be signed in to change notification settings - Fork 685
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
Supplant MagentoRouteHandler #1966
Changes from all commits
fec971c
7a7ef14
5882424
701f7bf
c9b9031
cabd8ec
9beca88
e1c7551
79b9b33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
const addToCache = async (...urls) => { | ||
if (!window.caches) { | ||
throw new Error( | ||
'Current environment does not support CacheStorage at window.caches.' | ||
); | ||
} | ||
|
||
const cache = await window.caches.open( | ||
`workbox-runtime-${location.origin}/` | ||
); | ||
|
||
await cache.addAll(urls); | ||
}; | ||
|
||
export default addToCache; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import resolveUnknownRoute from '../../Router/resolveUnknownRoute'; | ||
|
||
export const INTERNAL_ERROR = 'INTERNAL_ERROR'; | ||
export const NOT_FOUND = 'NOT_FOUND'; | ||
|
||
const getRouteComponent = async (apiBase, pathname) => { | ||
// At build time, `fetchRootComponent` is injected as a global. | ||
// Depending on the environment, this global will be either an | ||
// ES module with a `default` property, or a plain CJS module. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very clear, thank you. |
||
const fetchRoot = | ||
'default' in fetchRootComponent | ||
? fetchRootComponent.default | ||
: fetchRootComponent; | ||
|
||
try { | ||
// try to resolve the route | ||
// if this throws, we essentially have a 500 Internal Error | ||
const resolvedRoute = await resolveUnknownRoute({ | ||
apiBase, | ||
route: pathname | ||
}); | ||
|
||
// urlResolver query returns null if a route can't be found | ||
if (!resolvedRoute) { | ||
throw new Error('404'); | ||
} | ||
|
||
const { type, id } = resolvedRoute; | ||
// if resolution and destructuring succeed but return no match | ||
// then we have a straightforward 404 Not Found | ||
if (!type || !id) { | ||
throw new Error('404'); | ||
} | ||
|
||
// at this point we should have a matching RootComponent | ||
// if this throws, we essentially have a 500 Internal Error | ||
const component = await fetchRoot(type); | ||
|
||
// associate the matching RootComponent with this location | ||
return { | ||
component, | ||
id, | ||
pathname, | ||
type | ||
}; | ||
} catch (e) { | ||
const routeError = e.message === '404' ? NOT_FOUND : INTERNAL_ERROR; | ||
|
||
console.error(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for this, too. |
||
|
||
// we don't have a matching RootComponent, but we've checked for one | ||
// so associate the appropriate error case with this location | ||
return { pathname, routeError }; | ||
} | ||
}; | ||
|
||
export default getRouteComponent; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export { INTERNAL_ERROR, NOT_FOUND } from './getRouteComponent'; | ||
export { useMagentoRoute } from './useMagentoRoute'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import { useEffect, useRef, useState } from 'react'; | ||
import { useLocation } from 'react-router-dom'; | ||
import { useApolloClient } from '@apollo/react-hooks'; | ||
|
||
import addToCache from './addToCache'; | ||
import getRouteComponent from './getRouteComponent'; | ||
|
||
const IS_LOADING = { isLoading: true }; | ||
|
||
export const useMagentoRoute = () => { | ||
const [componentMap, setComponentMap] = useState(new Map()); | ||
const { apiBase } = useApolloClient(); | ||
const { pathname } = useLocation(); | ||
const isMountedRef = useRef(false); | ||
|
||
const routeData = componentMap.get(pathname); | ||
|
||
// ask Magento for a RootComponent that matches the current pathname | ||
useEffect(() => { | ||
// avoid asking if we already know the answer | ||
const isKnown = componentMap.has(pathname); | ||
|
||
// ask again following a prior failure | ||
const isNotFound = isKnown && componentMap.get(pathname).id === -1; | ||
const shouldReload = isNotFound && navigator.onLine; | ||
|
||
if (!isKnown || shouldReload) { | ||
getRouteComponent(apiBase, pathname).then( | ||
({ component, id, pathname, routeError }) => { | ||
// avoid setting state if unmounted | ||
if (!isMountedRef.current) { | ||
return; | ||
} | ||
|
||
// add the pathname to the browser cache | ||
addToCache(pathname); | ||
jimbo marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the old implementation we had a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that is here still. One of the other reviewers suggested that I remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, didn't realize we eliminated consoles in prod. |
||
|
||
// then add the pathname and result to local state | ||
setComponentMap(prevMap => { | ||
jimbo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const nextMap = new Map(prevMap); | ||
const nextValue = routeError | ||
? { hasError: true, routeError } | ||
: { component, id }; | ||
|
||
return nextMap.set(pathname, nextValue); | ||
}); | ||
} | ||
); | ||
} | ||
}, [apiBase, componentMap, pathname]); | ||
|
||
useEffect(() => { | ||
isMountedRef.current = true; | ||
|
||
return () => { | ||
isMountedRef.current = false; | ||
}; | ||
}, []); | ||
|
||
return routeData || IS_LOADING; | ||
}; |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to see caching but can we delegate this work to the SW? 2 reasons:
cacheName
can change in the future and having it hardcoded here can be a potential cause for bugs. We can leave this to the SW and if thecacheName
changes in the future workbox will take care of it.You can use sendMessageToSw to send a message along with the list of URL you want to cache and have a handler on the SW do the caching part.
For instance:
On the UI:
In SW:
Check out this example:
pwa-studio/packages/peregrine/lib/talons/ProductImageCarousel/useProductImageCarousel.js
Line 36 in 21aadbc
pwa-studio/packages/venia-concept/src/ServiceWorker/Utilities/imageCacheHandler.js
Line 144 in 21aadbc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback, I agree with you on all points. I'd like to handle the change in a separate PR, though. This one is just a 1:1 refactor—nothing about
addToCache
has changed here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
addToCache
implementation is different here, though. The original implementation had some big problems, so I'm not about bringing it back, but I think this isn't populating the cache correctly. Can you confirm that it's actually caching and reusing theurlResolver
responses? I think this is just caching an app shell instead of a GraphQL response.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call me crazy, but did I do something other than just move this code into its own file?