Split the skeleton template loaders to separate primary and secondary data functions #2130
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
There was a problem hiding this comment.
While I like the convention to avoid sequential requests, this might make it a bit harder to create data dependencies: load primary data, then load secondary data using some primary data as argument. For example, get recommended products from a product.id in demo-store.
What about only extracting secondaryData and keeping primaryData in the original place so that the loader itself doesn't become a dummy function and you can pass information around? We keep the Promise.all in place but in the loader:
export async function loader(args: LoaderFunctionArgs) {
// Fire secondary data requests early to do them in parallel
// if they don't depend on primary data.
const secondaryData = loadSecondaryData(args);
// Load primary data in parallel:
const [header] = await Promise.all([
context.storefront.query(HEADER_QUERY, {
cache: context.storefront.CacheLong(),
variables: {
headerMenuHandle: 'main-menu', // Adjust to your header menu handle
},
}),
// Add other queries here, so that they are loaded in parallel
]);
// Delay secondary data until here if it depends on primary data,
// or it's loaded conditionally depending on primary data:
if (!header) throw new Response();
const secondaryData = loadSecondaryData(args, header);
// You can keep looking at loader's return statement to see primary and static data:
return {
...secondaryData,
header,
publicStoreDomain: context.env.PUBLIC_STORE_DOMAIN,
}
}Just trying to find something that feels Remixy and flexible but still gives enough nuances to avoid waterfalls 🤔
|
@frandiox I like it. Will discuss in the hangout today and report back. |
|
@frandiox a few thoughts after discussion:
|
blittle
force-pushed
the
bl-template-loaders
branch
2 times, most recently
from
6aa8f3c
to
a03a5d3
Compare
There was a problem hiding this comment.
Should we add some messaging in loadDeferredData about not throwing in that function? Like, avoid doing things like throw Error or throw redirect. Plus, perhaps adding .catch to the promises?
Otherwise we would have ugly unhandled rejections that might interfere with critical data 🤔 -- unsure about this though, can never remember what's the actual behavior around this in Remix loaders.
blittle
force-pushed
the
bl-template-loaders
branch
from
a03a5d3
to
43bb308
Compare
|
@frandiox updated! Thx for the review! |
There was a problem hiding this comment.
I think this convention is great but it makes an issue with unhandled rejections a bit worse because we are firing "uncontrolled" requests before starting streaming.
For example, I'm testing with an artificial error for a deferred query. Specifically, adding the following to this line (make sure you build/dev packages/hydrogen):
if (query?.includes('footer')) {
throwErrorWithGqlLink({
...errorOptions,
errors: [{message: 'artificial error for footer query'}],
});
}Which, when loading / it shows this correctly from the root loader:
The page is also rendered in the browser without the footer. However, the tab hangs in some kind of infinite loop of errors in the dev tools:
This type of issue is now worse because deferred requests might throw even before Remix starts streaming (because we call loadDeferredData before loadCriticalData).
I think we should adopt a safer strategy by catching errors in deferred data requests and make them nullable in the frontend:
The good news is that now that deferred data is all located in 1 function, it's easier to add the convention of .catch(() => null) or whatever we choose there!
We could print errors automatically (we already do this for GraphQL errors but not for network errors) so that they don't even need to call console.error?
Or we brainstorm other things like a storefront.safeQuery(...) that doesn't throw or something. But kind of nice to keep it without abstraction as .catch(() => null) I think?
|
interesting...I am encountering the same "infinite loop of errors in the dev tools" while doing something for Single Fetch. How timely! |
|
@frandiox I think there's something unrelated causing the inf loop. I update |
@blittle Yes, the error is probably not introduced here. But I think these changes increase the chance of showing it because we are not handling thrown errors in deferred data... I think? Say there's an asynchronous error thrown from
I'm not sure if this is true? Maybe I'm misunderstanding it. If this is a 400-500 error or a network error, we don't return anything from That's why I'm suggesting we add -- GraphQL errors, on the other hand (status 200) will get serialized and there's no issue here. But the
We ensure sync errors don't happen in |
|
First of all: being more defensive and adding catch for error is always a good thing 👍 But there are defin something going on with the remix upgrade in meantime as well. Pre Remix 2.9 Remix 2.9 Much less scary then the infinite loop error that crash your browser. Since we cant really catch ALL the error ever, I am putting myself on the task to figure out a better way to handle this over all in our application. |
|
Issue logged with Remix: remix-run/remix#9553 |
|
@frandiox I assumed this wouldn't be an issue because we added this https://github.com/Shopify/hydrogen/pull/1318/files and subsquently this: https://github.com/Shopify/hydrogen/blob/main/packages/hydrogen/src/utils/callsites.ts#L43 So all promises from our API clients should handle uncaught promise exceptions? |
Being defensive is good, but adding catch statements in the wrong layer prevents the error from being consumed. |
|
Additionally, I can't get the express/node example to break with an uncaught promise exception from a loader: export async function loader() {
return defer({
test: new Promise((resolve) => {
throw new Error('it broke');
}),
});
}
export default function Index() {
const {test} = useLoaderData();
return (
<>
<Await resolve={test} errorElement={<div>another broke</div>}>
{({test}) => <div>{test}</div>}
</Await>
</>
);
}It seems to me that remix now immediately adds a catch to promises passed to |
That
I guess we need to decide what's the right layer to consume the error. I'm not sure if sending an error for secondary data it to the browser is much better than a null, at least in production... I don't think the fallback component normally takes the error into account? Might be wrong though!
The situation would be more like this: 
However, it looks like in latest Remix and workerd, unhandled rejections are indeed getting caught: 
So maybe we're good to go if we add |
… data functions The goal here is to make the template more self documenting: 1. Force people to consider up front what to defer and what not to 2. Default to a `Promise.all(`) and a comment within queries to encourage parallel requests 3. The `secondaryData` function is _not_ async, so people can't await.
blittle
force-pushed
the
bl-template-loaders
branch
from
db42c20
to
e673c58
Compare
The goal here is to make the template more self documenting:
Promise.all() and a comment within queries to encourage parallel requestssecondaryDatafunction is not async, so people can't await.Checklist