Add storefront api typing to hydrogen framework

[frehner]

Fix some issues that came up as a result of the better typing.

The demo-store turns out has a bunch of TS angry squigglies because of this update, but I'm not sure I have it in me to update them all right now. :/

Closes #2107

---

Before submitting the PR, please make sure you do the following:

• [x] Read the Contributing Guidelines
• [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
• [ ] Update docs in this repository according to your change
• [x] Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

[frehner]

@Shopify/hydrogen This is ready for review again

[shopify[bot]]

Oxygen deployed a preview of your bl-template-loaders branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM
Skeleton (skeleton.hydrogen.shop)	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	June 5, 2024 4:56 PM

Learn more about Hydrogen's GitHub integration.

[blittle]

@frandiox I like it. Will discuss in the hangout today and report back.

[blittle]

@frandiox a few thoughts after discussion:

1. Keeping primaryData and secondaryData in separate functions makes them slightly more self documenting and obvious.
2. Adding an extra function also creates more indirection.
3. Either route we go, seems maybe there is question about the function naming secondaryData and primaryData.

[blittle]

@frandiox updated! Thx for the review!

[michenly]

interesting...I am encountering the same "infinite loop of errors in the dev tools" while doing something for Single Fetch. How timely!

[blittle]

@frandiox I think there's something unrelated causing the inf loop. I update Footer.tsx with <Await resolve={footerPromise} errorElement={<h1>it broke</h1>}> and it goes away. Additionally, swallowing the async query error is probably undesirable, because the dev might still want to do something with it in the browser. The fail state still gets serialized to the browser. The real issue here is if there's a synchronous error in loadDeferredData. But in that case, adding a simple .catch() isn't going to help. The only way to make sure that doesn't happen would be to wrap the whole thing in a sync try catch 🤔

[frandiox]

I think there's something unrelated causing the inf loop. I update Footer.tsx with <Await resolve={footerPromise} errorElement={<h1>it broke</h1>}> and it goes away.

@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 loadDeferredData while we await for loadCriticalData. Remix didn't have the chance to grab the promises of the deferred data yet because we didn't return from the loader (still awaiting loadCriticalData). What happens in this situation? I think it depends on the JS runtime (and its version), but it could even exit the process in something like Node.js .

Additionally, swallowing the async query error is probably undesirable, because the dev might still want to do something with it in the browser. The fail state still gets serialized to the browser.

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 storefront.query... we throw instead. If we throw and the user is not calling .catch, it will turn into an unhandled rejection that won't go to the browser.

That's why I'm suggesting we add .catch and return null instead, so that the browser gets the null and the user can do something with it (hide UI or show a fallback). Of course we could return a "default query result" from the .catch instead of null but I'm not sure if that's useful in a template?

-- GraphQL errors, on the other hand (status 200) will get serialized and there's no issue here. But the .catch won't swallow those because they are returned, not thrown.

The real issue here is if there's a synchronous error in loadDeferredData. But in that case, adding a simple .catch() isn't going to help. The only way to make sure that doesn't happen would be to wrap the whole thing in a sync try catch 🤔

We ensure sync errors don't happen in storefront.query source code. I think that should be enough? But the issue I'm referring to is asynchronous but still thrown.

[michenly]

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. I just went back to pre Remix 2.9 of our repo, create the same error and gets an Application Error in the browser.

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.

[michenly]

Issue logged with Remix: https://github.com/remix-run/remix/issues/9553 I think I will revert my PR https://github.com/Shopify/hydrogen/pull/2152

[blittle]

@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?

[blittle]

First of all: being more defensive and adding catch for error is always a good thing 👍

Being defensive is good, but adding catch statements in the wrong layer prevents the error from being consumed.

[blittle]

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 defer. If I add one outside the loader, the process does crash. So far in my testing it appears the best solution is to let the promise errors serialize to the browser and handle them there.

[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

That .catch still throws an error though, so the unhandled piece should continue 🤔

Being defensive is good, but adding catch statements in the wrong layer prevents the error from being consumed.

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!

Additionally, I can't get the express/node example to break with an uncaught promise exception from a loader:

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 errorElement to the suspense as you mentioned somewhere else?