Use new parameters for app management create mutations #4411

gordonhirsch · 2024-09-03T20:59:22Z

WHY are these changes introduced?

Part 2 of https://github.com/Shopify/develop-app-foundations/issues/1050

This PR adapts to changes in the. App Management API introduced in part 1 of the sequence of PRs.

WHAT is this pull request doing?

The new AppVersionInput graphql type is used in the App Management client as a variable for in the app/version create mutations.

On deploy, if a bundle has been uploaded, only that bundle URL is specified as sourceUrl in AppVersionInput. That means that the bundle's manifest is the sole source of app module information like config, etc.

If no bundle has been uploaded, then name and app module information is passed in the source field of AppVersionInput, similar to existing (now-deprecated) API.

It doesn't appear that there are existing tests to cover the deploy logic in the app management client, so I added a few new ones to test deploys and this PR in particular.

How to test your changes?

Running app deploy for a new app will exercise all of the changes in this PR. Note that there is currently an unrelated issue where an app's initial deploy will fail at the CLI level because of a switch to using the partners client for some reason. In this case, the app and initial version are still created correctly.

Testing can be run against production once part 1 has deployed, or against a spin instance running that PR's branch.

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

github-actions · 2024-09-03T20:59:40Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-inner-loop

github-actions · 2024-09-03T21:03:32Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	73.28% (+0.19% 🔼)	8432/11507
🟡	Branches	69.79% (+0.2% 🔼)	4112/5892
🟡	Functions	72.04% (+0.17% 🔼)	2190/3040
🟡	Lines	73.62% (+0.19% 🔼)	7978/10837

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / output.ts	77.39% (-0.19% 🔻)	76.19% (+0.81% 🔼)	63.64%	77.06% (-0.21% 🔻)

Test suite run success

1898 tests passing in 861 suites.

Report generated by 🧪jest coverage report action from b40cdf6

gordonhirsch · 2024-09-04T18:03:40Z

.../app/src/cli/utilities/developer-platform-client/app-management-client/graphql/create-app.ts

@@ -1,9 +1,10 @@
 import {JsonMapType} from '@shopify/cli-kit/node/toml'
 import {gql} from 'graphql-request'

+// eslint-disable-next-line @shopify/cli/no-inline-graphql


We can start using graphql-codegen once the app management schema is fully in sync with the definitions here.

gordonhirsch · 2024-09-04T18:04:03Z

.../cli/utilities/developer-platform-client/app-management-client/graphql/create-app-version.ts

@@ -1,9 +1,10 @@
 import {JsonMapType} from '@shopify/cli-kit/node/toml'
 import {gql} from 'graphql-request'

+// eslint-disable-next-line @shopify/cli/no-inline-graphql


We can start using graphql-codegen once the app management schema is fully in sync with the definitions here.

github-actions · 2024-09-05T18:25:21Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

amcaplan · 2024-09-05T20:44:38Z

packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts

@@ -188,3 +193,200 @@ describe('versionDeepLink', () => {
    expect(got).toEqual('https://dev.shopify.com/dashboard/1/apps/2/versions/3')
  })
 })
+
+describe('deploy', () => {


Personal opinion.

These tests seem like lengthy restatements of the type signatures, and personally I don't find that they increase my confidence in the code beyond the type signature itself. It's just another thing to change if we need to change the type signature later.

I do see the value in checking the logic in generation of the inputs, since it varies based on inputs to deploy. So maybe the tests could be significantly cut down by just focusing on that piece of it, verifying that the inputs are generated correctly. But I think appManagementRequest can just be a mock, since the only real concern is that the inputs are right.

Hmm, ok, I could simplify the mocked response. There is some post-request code to map the response that it was meant to verify. But it's really an example of trying to do a bit to improve the tests while in there, and not strictly related to this PR.

I've simplified the tests somewhat in the latest commit (843b3db) . If you had something more in mind, let's talk about it.

amcaplan

I'm still debating internally whether my previous comment was correct - is it better to have less detail here? - maybe @isaacroldan can offer an opinion.

I can say for myself that clearing out some of the details made it easier for me to understand the goals of each test, hence a bit more detailed feedback this round.

amcaplan · 2024-09-08T13:26:29Z

.../cli/utilities/developer-platform-client/app-management-client/graphql/create-app-version.ts

+    sourceUrl?: string
+    source?: {


It looks like we want to enforce having either source or sourceUrl. I believe a union type would be great for this. Something like:

interface SourceUrlVersion { sourceUrl: string } interface SourceVersion { source: { // ... all the properties } } export interface CreateAppVersionMutationVariables { // ... other properties version: SourceUrlVersion | SourceVersion }

By the way, I don't know if this kind of union type is, or even can be, reflected in our generated GraphQL API docs. Something to consider when we decide whether to move over this mutation to have auto-generated types.

Good point. I think we'd want to use a GraphQL union type in the schema if we want to use it here. I'll need to revisit why it is not that way in the schema 🤔

It looks like GraphQL unions are not support for input types, but I have changed the code here to use the union you suggested, at least for now. The types have have also changed to match the latest server change to use a manifests everywhere instead of a separate app version source type.

packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts

packages/app/src/cli/utilities/developer-platform-client/app-management-client.test.ts

isaacroldan · 2024-09-09T10:48:47Z

packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts

-    if (brandingModule) {
-      updatedName = JSON.parse(brandingModule.config).name
+    let versionInput
+    if (bundleUrl) {


Even if there are no assets, we want to build the manifest and upload it to GCS no? so there will always be a bundleUrl? In which cases can we deploy without a bundle URL?

If/when we need to store a manifest in GCS, I think it would be easy to generate one on the back end for the no asset case. In general I think it is worthwhile to have an API for creating app versions that does not require the client to make 3 network calls for a new app version (request bundle url, upload bundle, create the version).

isaacroldan · 2024-09-09T10:50:57Z

packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts

+      versionInput = {
+        source: {
+          name: updatedName,
+          appModules: (appModules ?? []).map((mod) => {
+            return {


Should we force to always use a bundleURL and not accept versionInput with a an appModule array? This way we can be 100% sure that all versions have a manifest and we don't need to support two formats for the API here.

We can do it that way, but to me, it seems a bit wasteful to require a separate bundle upload when there are no assets in the app.

It could seem that way, but is a more unified API and the code would be simpler in both CLI and backend, maybe is worth the extra resources for that? 🤔

The latest server change unifies the API a bit more. Instead of a separate source type, it takes the actual manifest JSON when the app version is directly specified. The code in this PR has been updated accordingly. The back end will still have the responsibility of storing the manifest, but I think the API change removes the biggest concerns and is a reasonable compromise.

isaacroldan · 2024-09-26T13:21:58Z

packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts

+              handle: mod.handle,
+              config: JSON.parse(mod.config),
+            }
+          }),


the App object already has a function to generate the manifest, I don't think we should duplicate it here.
This function could either receive the built manifest or the AppInterface object and use it to build generate the manifest.

This makes a lot of sense to me, but since it involves changing an interface, I wonder if it would be best tackled in a separate PR. There's a sequence of changes we are trying to make now to unblock the consistent dev experience work.

I'm ok with that, as long as we don't forget about it! :)

github-actions · 2024-10-27T03:39:31Z

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

gordonhirsch force-pushed the use-updated-creation-api branch 2 times, most recently from d66609e to 9849c5b Compare September 4, 2024 17:53

gordonhirsch commented Sep 4, 2024

View reviewed changes

gordonhirsch force-pushed the use-updated-creation-api branch 3 times, most recently from 694fb8c to d78ea2f Compare September 5, 2024 15:20

gordonhirsch marked this pull request as ready for review September 5, 2024 18:25

gordonhirsch requested review from amcaplan and gonzaloriestra September 5, 2024 18:25

gordonhirsch requested a review from isaacroldan September 5, 2024 18:25

amcaplan reviewed Sep 5, 2024

View reviewed changes

amcaplan reviewed Sep 8, 2024

View reviewed changes

isaacroldan reviewed Sep 9, 2024

View reviewed changes

gordonhirsch marked this pull request as draft September 10, 2024 14:05

gordonhirsch added 3 commits September 24, 2024 20:45

Use new parameters for app management create mutations

4a8ebd8

simplify deploy tests

9b6027c

Use manifest for direct specification of app version source

f79589d

gordonhirsch force-pushed the use-updated-creation-api branch from 843b3db to f79589d Compare September 25, 2024 14:28

address additional review comments

b40cdf6

gordonhirsch marked this pull request as ready for review September 25, 2024 15:01

gordonhirsch requested a review from amcaplan September 25, 2024 15:10

isaacroldan reviewed Sep 26, 2024

View reviewed changes

github-actions bot added the no-pr-activity label Oct 27, 2024

github-actions bot closed this Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new parameters for app management create mutations #4411

Use new parameters for app management create mutations #4411

gordonhirsch commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 •

edited

Loading

gordonhirsch Sep 4, 2024

gordonhirsch Sep 4, 2024

github-actions bot commented Sep 5, 2024

amcaplan Sep 5, 2024

gordonhirsch Sep 5, 2024

gordonhirsch Sep 6, 2024

amcaplan left a comment

amcaplan Sep 8, 2024

gordonhirsch Sep 9, 2024

gordonhirsch Sep 25, 2024

isaacroldan Sep 9, 2024

gordonhirsch Sep 9, 2024

isaacroldan Sep 9, 2024

gordonhirsch Sep 9, 2024

isaacroldan Sep 10, 2024

gordonhirsch Sep 25, 2024

isaacroldan Sep 26, 2024

gordonhirsch Sep 26, 2024

isaacroldan Sep 26, 2024

github-actions bot commented Oct 27, 2024

Use new parameters for app management create mutations #4411

Use new parameters for app management create mutations #4411

Conversation

gordonhirsch commented Sep 3, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

github-actions bot commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcaplan 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

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

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

github-actions bot commented Oct 27, 2024

gordonhirsch commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading