-
Notifications
You must be signed in to change notification settings - Fork 130
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
Introduce dev dash webhooks API #4884
base: main
Are you sure you want to change the base?
Conversation
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1950 tests passing in 888 suites. Report generated by 🧪jest coverage report action from 14e92a0 |
We detected some changes at packages/*/src and there are no updates in the .changeset. |
552699e
to
14e92a0
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationspackages/cli-kit/dist/public/node/api/webhooks.d.tsimport { Variables } from 'graphql-request';
import { TypedDocumentNode } from '@graphql-typed-document-node/core';
/**
* Executes an org-scoped GraphQL query against the App Management API.
* Uses typed documents.
*
* @param query - GraphQL query to execute.
* @param token - Partners token.
* @param variables - GraphQL variables to pass to the query.
* @returns The response of the query of generic type <T>.
*/
export declare function webhooksRequest<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables): Promise<TResult>;
Existing type declarationsWe found no diffs with existing type declarations |
repo: 'shopify', | ||
pathToFile: 'areas/core/shopify/db/graphql/webhooks_schema_unstable_public.graphql', | ||
localPath: './packages/app/src/cli/api/graphql/webhooks/webhooks_schema.graphql', | ||
branch: 'dev-dash-webhooks-api', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes tophating possible for now (see the Core branch https://github.com/Shopify/shopify/pull/554877). Once the Core PR has been merged, I'll change this for dd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with caveat around your other PR merging and the branch name in codegen being updated.
@@ -80,15 +80,17 @@ | |||
"{projectRoot}/src/cli/api/graphql/business-platform-destinations/generated/**/*.ts", | |||
"{projectRoot}/src/cli/api/graphql/business-platform-organizations/generated/**/*.ts", | |||
"{projectRoot}/src/cli/api/graphql/app-dev/generated/**/*.ts", | |||
"{projectRoot}/src/cli/api/graphql/app-management/generated/**/*.ts" | |||
"{projectRoot}/src/cli/api/graphql/app-management/generated/**/*.ts", | |||
"{projectRoot}/src/cli/api/graphql/webhooks/generated/**/*.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we had a nicer way to do this but all the plumbing looks great 👌
headers: '{}', | ||
success: true, | ||
userErrors: [], | ||
samplePayload: result.cliTesting?.samplePayload ?? '{}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of ?.
here. Should cliTesting
always be present, is it an error if its not? Maybe the schema should be adjusted?
query publicApiVersions { | ||
publicApiVersions { | ||
displayName | ||
handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given handle
is the only thing we use of this query, should we limit what we fetch to that?
}) | ||
}) | ||
|
||
describe('sendSampleWebhook', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe a thing relevant to test here is the mapping of input
to the mutation variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree w/ Shaun's comments otherwise LGTM
WHY are these changes introduced?
See https://github.com/Shopify/shopify/pull/554877 for the Core implementation and tophat instructions
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist