Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add context.storefront.mutate #193

Merged
merged 13 commits into from
Nov 17, 2022
Merged

Add context.storefront.mutate #193

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
eb52537
Change storefront.query API
frandiox Nov 15, 2022
3df7077
Update storefront.query calls
frandiox Nov 15, 2022
d26c175
Override LoaderFunction, ActionFunction and ActionArgs types
frandiox Nov 15, 2022
3f9fd9a
Update queries to use storefront.query
frandiox Nov 15, 2022
dc4e941
Check customerUserErrors in storefront.mutate
frandiox Nov 15, 2022
b1ac266
Use StorefrontApiError constructor for payload errors
frandiox Nov 15, 2022
1ecab1a
Throw conditionally
frandiox Nov 15, 2022
d00969b
Update mutations to use storefront.mutate
frandiox Nov 15, 2022
51dae54
Use ErrorConstructor type
frandiox Nov 16, 2022
c85263a
Move data error handling to user app
frandiox Nov 16, 2022
fb93c92
Fix type name
frandiox Nov 16, 2022
6217e9a
Assert query type
frandiox Nov 16, 2022
b76b6fb
Merge branch 'main' into fd-storefront-mutate
frandiox Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions packages/hydrogen-remix/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
import type {HydrogenContext} from '@shopify/hydrogen';
import type {Params} from '@remix-run/react';
import type {AppData, DataFunctionArgs} from '@remix-run/oxygen';

export * from '@remix-run/oxygen';
export {createRequestHandler} from './server';
export * from '@shopify/hydrogen';

export type LoaderArgs = {
request: Request;
params: Params;
export type LoaderArgs = DataFunctionArgs & {
context: HydrogenContext;
};

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;
}
Comment on lines +12 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

32 changes: 26 additions & 6 deletions packages/hydrogen/storefront.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export type CreateStorefrontClientOptions = {
};

type StorefromCommonOptions = {
frandiox marked this conversation as resolved.
Show resolved Hide resolved
variables: ExecutionArgs['variableValues'];
variables?: ExecutionArgs['variableValues'];
headers?: HeadersInit;
};

Expand Down Expand Up @@ -69,6 +69,10 @@ const shouldCacheResponse = (body: any) => {
}
};

export class StorefrontApiError extends Error {}
export const isStorefrontApiError = (error: any) =>
error instanceof StorefrontApiError;

export function createStorefrontClient(
clientOptions: StorefrontClientProps,
{
Expand All @@ -90,7 +94,7 @@ export function createStorefrontClient(
defaultHeaders[STOREFRONT_REQUEST_GROUP_ID_HEADER] = requestGroupId;
if (buyerIp) defaultHeaders[STOREFRONT_API_BUYER_IP_HEADER] = buyerIp;

async function getStorefrontData<T>({
async function callStorefrontApi<T>({
query,
mutation,
variables,
Expand Down Expand Up @@ -143,14 +147,27 @@ export function createStorefrontClient(

const {data, errors} = body as StorefrontApiResponse<T>;

if (errors) throwError(response, errors);
if (errors?.length) throwError(response, errors, StorefrontApiError);
else if (mutation && data) {
const mutationErrors = Object.values(data)
.map((value: any) => value?.customerUserErrors?.[0]?.message)
.filter(Boolean);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@juanpprieto juanpprieto Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@frandiox frandiox Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 馃憤

Copy link
Contributor

@juanpprieto juanpprieto Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


if (mutationErrors.length)
throwError(response, mutationErrors, StorefrontApiError);
}

return data as T;
}

return {
storefront: {
query: getStorefrontData,
query: <T>(
query: string,
payload?: StorefromCommonOptions & {cache?: CachingStrategy},
) => callStorefrontApi<T>({...payload, query}),
mutate: <T>(mutation: string, payload?: StorefromCommonOptions) =>
callStorefrontApi<T>({...payload, mutation}),
getPublicTokenHeaders,
getPrivateTokenHeaders,
getStorefrontApiUrl,
Expand Down Expand Up @@ -181,6 +198,7 @@ export function createStorefrontClient(
function throwError<T>(
response: Response,
errors: StorefrontApiResponse<T>['errors'],
ErrorConstructor: any = Error,
frandiox marked this conversation as resolved.
Show resolved Hide resolved
) {
const reqId = response.headers.get('x-request-id');
const reqIdMessage = reqId ? ` - Request ID: ${reqId}` : '';
Expand All @@ -191,8 +209,10 @@ function throwError<T>(
? errors
: errors.map((error) => error.message).join('\n');

throw new Error(errorMessages + reqIdMessage);
throw new ErrorConstructor(errorMessages + reqIdMessage);
}

throw new Error(`API response error: ${response.status}` + reqIdMessage);
throw new ErrorConstructor(
`API response error: ${response.status}` + reqIdMessage,
);
}
Loading