Add `context.storefront.mutate` #193

frandiox · 2022-11-15T08:52:01Z

As mentioned in the team sync today, this is a suggestion to change the signature of our storefront.query function.

Before:

storefront.query({query: '', variables: {}, cache: CacheLong()});
storefront.query({mutation: '', variables: {}});

After

storefront.query('', {variables: {}, cache: CacheLong()});
storefront.mutate('', {variables: {}});

This seems more straightforward and needs simpler types. However, if we don't like this option I can revert the syntax and keep the other changes 👍

Aside from that, this changes the demo-store to start using context.storefront and remove the old getStorefrontData utility.
It also fixes LoaderArgs, ActionArgs, ActionFunction and LoaderFunction types.

It's easier to look at this PR commit by commit.

frandiox · 2022-11-15T08:56:07Z

packages/hydrogen/storefront.ts

+      const mutationErrors = Object.values(data)
+        .map((value: any) => value?.customerUserErrors?.[0]?.message)
+        .filter(Boolean);


Does this look correct? I'm not very familiar with customerUserErrors in mutations.

I think this looks right. @frehner what do you think?

I think the client should only handle connection errors e.g (errors.length) internally, and should return data unaltered to let the mutation callee handle the graphql errors on the action (so that we can display them on the UI)

Because cart and customer mutations return errors under different keys cartUserErrors vs customerUserErrors, I like to alias both of these as errors.

This way in an action we can do:

const {cart, errors} = await storefront.mutation(cartMutation)
const {customer, errors} = await storefront.mutation(customerMutation)

See:
cart mutation errors
customer mutation error

Yeah it's unfortunate that it appears that there are different error properties on different mutations. Ugh.

Ugh I see. The problem is that storefront.query right now only returns data... We would need to change the signature from const data = to const {data, errors} =. This means we'd need to start adding .then(payload => payload.data) when using storefront.query in defer

Nevermind, for some reason I thought the customerUserErrors array was in errors instead of data. I'll update the code to handle the customerUserErrors on the user app side 👍

I don't think you need to change it because both customerUserErrors and cartUserErrors are nested within the data object

There's two types of errors:

fetch errors: You can deal with these in the client (as you are doing with errors.length

graphql errors: We should deal with these on the action/loader

What may have confused you is that I called customerUserErrors and cartUserErrors as errors. But what I meant is this:

const data = await storefront.mutation(cartMutation) const {cart, cartUserErrors: errors} = data;

Does that make sense?

Ah, I modified my comment earlier without refreshing the page so I didn't see your last comment @juanpprieto , but yeah it makes sense, thanks!

blittle · 2022-11-15T14:46:16Z

packages/hydrogen-remix/index.ts

+export interface LoaderFunction {
+  (args: LoaderArgs): Promise<Response> | Response | Promise<AppData> | AppData;
+}
+
+export type ActionArgs = DataFunctionArgs & {
+  context: HydrogenContext;
+};
+
+export interface ActionFunction {
+  (args: ActionArgs): Promise<Response> | Response | Promise<AppData> | AppData;
+}


Aren't these essentially the same as what Remix provides? Why not use their types instead?

I think is because we are modifying DataFunctionArgs['context']?

Yes we are modifying context to be HydrogenContext, that's the only reason.

juanpprieto

👍🏼

blittle

A few nit picky items. Great work Fran!

blittle · 2022-11-15T14:47:52Z

packages/hydrogen/storefront.ts

+      const mutationErrors = Object.values(data)
+        .map((value: any) => value?.customerUserErrors?.[0]?.message)
+        .filter(Boolean);


I think this looks right. @frehner what do you think?

packages/hydrogen/storefront.ts

blittle · 2022-11-15T14:50:56Z

templates/demo-store/app/data/index.ts

-}
-
-export async function getLayoutData(params: Params) {
+export async function getLayoutData(


Nit picky, but IMO we should get rid of this data index file, and just co-locate the query where it's used, in root.tsx

Yeah I want to do that but perhaps in a different PR more focused on refactoring?
In fact, I know JP is modifying data/index to change things in Cart so I think it's better if we do this later to minimize conflicts with his PR.

blittle · 2022-11-15T14:59:01Z

templates/demo-store/app/routes/account/__public/register.tsx

@@ -60,7 +61,7 @@ export const action: ActionFunction = async ({request, context, params}) => {
      },
    });
  } catch (error: any) {
-    if (error instanceof StorefrontApiError) {
+    if (isStorefrontApiError(error)) {


Interesting, exposing this as a utility function to easily identify storefront api errors 🤔

blittle · 2022-11-15T15:00:40Z

templates/demo-store/app/routes/pages/$pageHandle.tsx

@@ -14,8 +14,7 @@ export async function loader({params, context: {storefront}}: LoaderArgs) {
  invariant(params.pageHandle, 'Missing page handle');

  const {language} = getLocalizationFromLang(params.lang);
-  const {page} = await storefront.query<{page: Page}>({
-    query: PAGE_QUERY,
+  const {page} = await storefront.query<{page: Page}>(PAGE_QUERY, {


I like this change, query is required, so make it the first arg. Second arg is optional.

packages/hydrogen/storefront.ts

frehner · 2022-11-15T16:46:00Z

packages/hydrogen/storefront.ts

+      const mutationErrors = Object.values(data)
+        .map((value: any) => value?.customerUserErrors?.[0]?.message)
+        .filter(Boolean);


Yeah it's unfortunate that it appears that there are different error properties on different mutations. Ugh.

lordofthecactus

Good job @frandiox, I agree the separation feels better.

lordofthecactus · 2022-11-16T14:13:14Z

templates/demo-store/app/routes/account/__private/edit.tsx

@@ -101,7 +101,7 @@ export const action: ActionFunction = async ({request, context, params}) => {
    formDataHas(formData, 'newPassword') &&
      (customer.password = formData.get('newPassword') as string);

-    await updateCustomer({customerAccessToken, customer});
+    await updateCustomer(context, {customerAccessToken, customer});


Having to pass context so that I can use the storefront querying / mutations feels verbose, specially in so many functions here.

Yeah we are supposed to remove these functions and just have them inlined in loader and action, but that will come in another PR 👍

cartogram · 2022-11-17T03:00:17Z

packages/hydrogen/storefront.ts

+        query: string,
+        payload?: StorefrontCommonOptions & {cache?: CachingStrategy},
+      ) => {
+        query = minifyQuery(query);


Any value in providing an option for users to opt out of minifying for debugging, etc...

frandiox added 8 commits November 15, 2022 16:19

Change storefront.query API

eb52537

Update storefront.query calls

3df7077

Override LoaderFunction, ActionFunction and ActionArgs types

d26c175

Update queries to use storefront.query

3f9fd9a

Check customerUserErrors in storefront.mutate

dc4e941

Use StorefrontApiError constructor for payload errors

b1ac266

Throw conditionally

1ecab1a

Update mutations to use storefront.mutate

d00969b

frandiox requested review from blittle and a team November 15, 2022 08:52

frandiox commented Nov 15, 2022

View reviewed changes

blittle reviewed Nov 15, 2022

View reviewed changes

juanpprieto reviewed Nov 15, 2022

View reviewed changes

blittle approved these changes Nov 15, 2022

View reviewed changes

frehner reviewed Nov 15, 2022

View reviewed changes

frandiox added 3 commits November 16, 2022 17:49

Use ErrorConstructor type

51dae54

Move data error handling to user app

c85263a

Fix type name

fb93c92

github-actions bot deployed to preview November 16, 2022 09:11 View deployment

Assert query type

6217e9a

github-actions bot deployed to preview November 16, 2022 09:41 View deployment

Merge branch 'main' into fd-storefront-mutate

b76b6fb

github-actions bot deployed to preview November 16, 2022 10:00 View deployment

lordofthecactus approved these changes Nov 16, 2022

View reviewed changes

frandiox merged commit ca61d77 into main Nov 17, 2022

frandiox deleted the fd-storefront-mutate branch November 17, 2022 01:29

cartogram reviewed Nov 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `context.storefront.mutate` #193

Add `context.storefront.mutate` #193

frandiox commented Nov 15, 2022 •

edited

frandiox Nov 15, 2022

blittle Nov 15, 2022

juanpprieto Nov 15, 2022 •

edited

frehner Nov 15, 2022

frandiox Nov 16, 2022 •

edited

juanpprieto Nov 16, 2022 •

edited

frandiox Nov 16, 2022

blittle Nov 15, 2022

juanpprieto Nov 15, 2022

frandiox Nov 16, 2022

juanpprieto left a comment

blittle left a comment

blittle Nov 15, 2022

blittle Nov 15, 2022

frandiox Nov 16, 2022 •

edited

blittle Nov 15, 2022

blittle Nov 15, 2022

frehner Nov 15, 2022

lordofthecactus left a comment

lordofthecactus Nov 16, 2022

frandiox Nov 17, 2022

cartogram Nov 17, 2022

Add context.storefront.mutate #193

Add context.storefront.mutate #193

Conversation

frandiox commented Nov 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpprieto Nov 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frandiox Nov 16, 2022 • edited

Choose a reason for hiding this comment

juanpprieto Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpprieto left a comment

Choose a reason for hiding this comment

blittle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frandiox Nov 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordofthecactus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `context.storefront.mutate` #193

Add `context.storefront.mutate` #193

frandiox commented Nov 15, 2022 •

edited

juanpprieto Nov 15, 2022 •

edited

frandiox Nov 16, 2022 •

edited

juanpprieto Nov 16, 2022 •

edited

frandiox Nov 16, 2022 •

edited