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.

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

Allow the graphql client to follow graphql spec and return errors #1867

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions packages/api-clients/admin-api-client/src/graphql/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export function createAdminApiClient({
customFetchApi,
logger,
isTesting,
matchGraphQLSpec,
}: AdminApiClientOptions): AdminApiClient {
const currentSupportedApiVersions = getCurrentSupportedApiVersions();

Expand Down Expand Up @@ -81,6 +82,7 @@ export function createAdminApiClient({
retries,
customFetchApi,
logger,
matchGraphQLSpec,
});

const getHeaders = generateGetHeaders(config);
Expand Down
1 change: 1 addition & 0 deletions packages/api-clients/admin-api-client/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ export type AdminApiClientOptions = Omit<
customFetchApi?: CustomFetchApi;
logger?: ApiClientLogger<AdminApiClientLogContentTypes>;
isTesting?: boolean;
matchGraphQLSpec?: boolean;
};
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export function createGraphQLClient({
customFetchApi = fetch,
retries = 0,
logger,
matchGraphQLSpec = false,
}: ClientOptions): GraphQLClient {
validateRetries({client: CLIENT, retries});

Expand All @@ -57,8 +58,8 @@ export function createGraphQLClient({
defaultRetryWaitTime: RETRY_WAIT_TIME,
});
const fetchFn = generateFetch(httpFetch, config);
const request = generateRequest(fetchFn);
const requestStream = generateRequestStream(fetchFn);
const request = generateRequest(fetchFn, matchGraphQLSpec);
const requestStream = generateRequestStream(fetchFn, matchGraphQLSpec);

return {
config,
Expand All @@ -76,11 +77,20 @@ export function generateClientLogger(logger?: Logger): Logger {
};
}

async function processJSONResponse<TData = any>(
async function processJSONResponse<
TData = any,
TMatchGraphQLSpec extends boolean = false,
>(
response: Response,
): Promise<ClientResponse<TData>> {
const {errors, data, extensions} = await response.json<any>();
matchGraphQLSpec: TMatchGraphQLSpec,
): Promise<ClientResponse<TData, TMatchGraphQLSpec>> {
const fullData = await response.json<any>();
if (matchGraphQLSpec) {
return fullData;
}

const {errors, data, extensions} = fullData;
// @ts-ignore
Copy link
Author

Choose a reason for hiding this comment

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

Couldn't get TS to not complain here - perhaps someone else might know why?

return {
...getKeyValueIfValid('data', data),
...getKeyValueIfValid('extensions', extensions),
Expand Down Expand Up @@ -148,9 +158,11 @@ function generateFetch(
};
}

function generateRequest(
function generateRequest<TMatchGraphQLSpec extends boolean>(
fetchFn: ReturnType<typeof generateFetch>,
matchGraphQLSpec: TMatchGraphQLSpec,
): GraphQLClient['request'] {
// @ts-ignore
Copy link
Author

Choose a reason for hiding this comment

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

Couldn't get TS to not complain here - perhaps someone else might know why?

return async (...props) => {
if (DEFER_OPERATION_REGEX.test(props[0])) {
throw new Error(
Expand All @@ -166,34 +178,43 @@ function generateRequest(
const contentType = response.headers.get('content-type') || '';

if (!response.ok) {
return {
errors: {
networkStatusCode: status,
message: formatErrorMessage(statusText),
response,
},
};
const message = formatErrorMessage(statusText);
return matchGraphQLSpec
? {errors: [{message}]}
: {
errors: {
networkStatusCode: status,
message,
response,
},
};
}

if (!contentType.includes(CONTENT_TYPES.json)) {
return {
errors: {
networkStatusCode: status,
message: formatErrorMessage(
`${UNEXPECTED_CONTENT_TYPE_ERROR} ${contentType}`,
),
response,
},
};
const message = formatErrorMessage(
`${UNEXPECTED_CONTENT_TYPE_ERROR} ${contentType}`,
);
return matchGraphQLSpec
? {errors: [{message}]}
: {
errors: {
networkStatusCode: status,
message,
response,
},
};
}

return processJSONResponse(response);
return processJSONResponse(response, matchGraphQLSpec);
} catch (error) {
return {
errors: {
message: getErrorMessage(error),
},
};
const message = getErrorMessage(error);
return matchGraphQLSpec
? {errors: [{message}]}
: {
errors: {
message,
},
};
}
};
}
Expand Down Expand Up @@ -272,10 +293,16 @@ function readStreamChunk(
};
}

function createJsonResponseAsyncIterator(response: Response) {
function createJsonResponseAsyncIterator<TMatchGraphQLSpec extends boolean>(
response: Response,
matchGraphQLSpec: TMatchGraphQLSpec,
) {
return {
async *[Symbol.asyncIterator]() {
const processedResponse = await processJSONResponse(response);
const processedResponse = await processJSONResponse(
response,
matchGraphQLSpec,
);

yield {
...processedResponse,
Expand Down Expand Up @@ -432,9 +459,11 @@ function createMultipartResponseAsyncInterator(
};
}

function generateRequestStream(
function generateRequestStream<TMatchGraphQLSpec extends boolean>(
fetchFn: ReturnType<typeof generateFetch>,
matchGraphQLSpec: TMatchGraphQLSpec,
): GraphQLClient['requestStream'] {
// @ts-ignore
return async (...props) => {
if (!DEFER_OPERATION_REGEX.test(props[0])) {
throw new Error(
Expand All @@ -457,7 +486,7 @@ function generateRequestStream(

switch (true) {
case responseContentType.includes(CONTENT_TYPES.json):
return createJsonResponseAsyncIterator(response);
return createJsonResponseAsyncIterator(response, matchGraphQLSpec);
case responseContentType.includes(CONTENT_TYPES.multipart):
return createMultipartResponseAsyncInterator(
response,
Expand Down
23 changes: 20 additions & 3 deletions packages/api-clients/graphql-client/src/graphql-client/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,32 @@ export interface ResponseErrors {
response?: Response;
}

export interface SpecResponseError {
message: string;
locations?: {line: number; column: number}[];
path?: string[];
extensions?: GQLExtensions;
}

export type GQLExtensions = Record<string, any>;

export interface FetchResponseBody<TData = any> {
export interface FetchResponseBody<
TData = any,
TMatchGraphQLSpec extends boolean = false,
> {
data?: TData;
extensions?: GQLExtensions;
headers?: Headers;
errors?: TMatchGraphQLSpec extends true ? SpecResponseError[] : never;
}

export interface ClientResponse<TData = any> extends FetchResponseBody<TData> {
errors?: ResponseErrors;
export interface ClientResponse<
TData = any,
TMatchGraphQLSpec extends boolean = false,
> extends Omit<FetchResponseBody<TData, TMatchGraphQLSpec>, 'errors'> {
errors?: TMatchGraphQLSpec extends true
? SpecResponseError[]
: ResponseErrors;
}

export interface ClientStreamResponse<TData = any>
Expand Down Expand Up @@ -81,6 +97,7 @@ export interface ClientOptions {
customFetchApi?: CustomFetchApi;
retries?: number;
logger?: Logger;
matchGraphQLSpec?: boolean;
}

export interface ClientConfig {
Expand Down
6 changes: 6 additions & 0 deletions packages/apps/shopify-api/future/flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ export interface FutureFlags {
* resolves a conflict with the default() method in that class.
*/
customerAddressDefaultFix?: boolean;

/**
* When enabled, the GraphQL client will match the spec and return `errors` on the response. When disabled, the client wiill throw errors instead.
* @default false
*/
matchGraphQLSpec?: boolean;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export class GraphqlClient {
logger: clientLoggerFactory(config),
userAgentPrefix: getUserAgent(config),
isTesting: config.isTesting,
matchGraphQLSpec: config.future?.matchGraphQLSpec,
});
}

Expand Down Expand Up @@ -127,7 +128,11 @@ export class GraphqlClient {
...(options as ApiClientRequestOptions<Operation, AdminOperations>),
});

if (response.errors) {
const matchGraphQLSpec =
this.graphqlClass().config.future?.matchGraphQLSpec ?? false;

if (response.errors && !matchGraphQLSpec) {
// @ts-ignore
const fetchResponse = response.errors.response;

throwFailedRequest(response, (options?.retries ?? 0) > 0, fetchResponse);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ import {AdminClientOptions} from './types';
// TODO: This is actually just a call through to the Shopify API client, but with a different API. We should eventually
// move this over to the library layer. While doing that, we should also allow the apiVersion to be passed into the REST
// client request calls.
export function graphqlClientFactory({
export function graphqlClientFactory<
TMatchGraphQLSpec extends boolean = false,
>({
params,
handleClientError,
session,
}: AdminClientOptions): GraphQLClient<AdminOperations> {
}: AdminClientOptions): GraphQLClient<AdminOperations, TMatchGraphQLSpec> {
return async function query(operation, options) {
const client = new params.api.clients.Graphql({
session,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,15 @@ export type AdminApiContext<
Resources extends ShopifyRestResources = ShopifyRestResources,
> =
FeatureEnabled<ConfigArg['future'], 'removeRest'> extends true
? AdminApiContextWithoutRest
: AdminApiContextWithRest<Resources>;
? AdminApiContextWithoutRest<
FeatureEnabled<ConfigArg['future'], 'matchGraphQLSpec'>
>
: AdminApiContextWithRest<
Resources,
FeatureEnabled<ConfigArg['future'], 'matchGraphQLSpec'>
>;

export interface AdminApiContextWithoutRest {
export interface AdminApiContextWithoutRest<TMatchGraphQLSpec extends boolean> {
/**
* Methods for interacting with the Shopify Admin GraphQL API
*
Expand Down Expand Up @@ -128,12 +133,13 @@ export interface AdminApiContextWithoutRest {
* export const authenticate = shopify.authenticate;
* ```
*/
graphql: GraphQLClient<AdminOperations>;
graphql: GraphQLClient<AdminOperations, TMatchGraphQLSpec>;
}

export interface AdminApiContextWithRest<
Resources extends ShopifyRestResources = ShopifyRestResources,
> extends AdminApiContextWithoutRest {
TMatchGraphQLSpec extends boolean = false,
> extends AdminApiContextWithoutRest<TMatchGraphQLSpec> {
/**
* Methods for interacting with the Shopify Admin REST API
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import {BasicParams} from '../../types';

import type {StorefrontContext} from '.';

export function storefrontClientFactory({
export function storefrontClientFactory<
TMatchGraphQLSpec extends boolean = false,
>({
params,
session,
}: {
params: BasicParams;
session: Session;
}): StorefrontContext {
}): StorefrontContext<TMatchGraphQLSpec> {
const {api} = params;

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {StorefrontOperations} from '@shopify/storefront-api-client';

import {GraphQLClient} from '../types';

export interface StorefrontContext {
export interface StorefrontContext<TMatchGraphQLSpec extends boolean = false> {
/**
* Method for interacting with the Shopify Storefront GraphQL API
*
Expand Down Expand Up @@ -74,5 +74,5 @@ export interface StorefrontContext {
* export const authenticate = shopify.authenticate;
* ```
*/
graphql: GraphQLClient<StorefrontOperations>;
graphql: GraphQLClient<StorefrontOperations, TMatchGraphQLSpec>;
}
14 changes: 9 additions & 5 deletions packages/apps/shopify-app-remix/src/server/clients/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,15 @@ export interface GraphQLQueryOptions<
export type GraphQLResponse<
Operation extends keyof Operations,
Operations extends AllOperations,
> = ResponseWithType<FetchResponseBody<ReturnData<Operation, Operations>>>;
TMatchGraphQLSpec extends boolean,
> = ResponseWithType<
FetchResponseBody<ReturnData<Operation, Operations>, TMatchGraphQLSpec>
>;

export type GraphQLClient<Operations extends AllOperations> = <
Operation extends keyof Operations,
>(
export type GraphQLClient<
Operations extends AllOperations,
TMatchGraphQLSpec extends boolean,
> = <Operation extends keyof Operations>(
query: Operation,
options?: GraphQLQueryOptions<Operation, Operations>,
) => Promise<GraphQLResponse<Operation, Operations>>;
) => Promise<GraphQLResponse<Operation, Operations, TMatchGraphQLSpec>>;
11 changes: 11 additions & 0 deletions packages/apps/shopify-app-remix/src/server/future/flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ export interface FutureFlags {
* @default false
*/
removeRest?: boolean;

/**
* When enabled, the GraphQL client will match the spec and return `errors` on the response. When disabled, the client wiill throw errors instead.
* @default false
*/
matchGraphQLSpec?: boolean;
}

// When adding new flags, use this format:
Expand All @@ -46,6 +52,11 @@ export interface ApiFutureFlags<_Future extends FutureFlagOptions> {
// We're currently hardcoding this flag to true in our settings, so we should propagate it here
lineItemBilling: true;
unstable_managedPricingSupport: true;
/**
* When enabled, the GraphQL client will match the spec and return `errors` on the response. When disabled, the client wiill throw errors instead.
* @default false
*/
matchGraphQLSpec?: boolean;
}

export type ApiConfigWithFutureFlags<Future extends FutureFlagOptions> =
Expand Down
Loading
Loading