cli icon indicating copy to clipboard operation
cli copied to clipboard

Remove metafields from specifications that don't need it

Open isaacroldan opened this issue 9 months ago β€’ 7 comments

WHY are these changes introduced?

Remove metafield support from schemas that don't actually need it.

WHAT is this pull request doing?

  • Keep metafields only in the extensions that really use them:
    • checkout_post_purchase
    • checkout_ui_extension
    • tax_calculation
    • ui_extension

How to test your changes?

  1. Create extensions of each type that supports metafields
  2. Add metafield configurations to these extensions
  3. Run shopify app deploy and verify that metafields are properly included in the extension payloads

Measuring impact

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

  • [x] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

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

isaacroldan avatar Apr 23 '25 13:04 isaacroldan

This stack of pull requests is managed by Graphite. Learn more about stacking.

isaacroldan avatar Apr 23 '25 13:04 isaacroldan

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
76.64% (+0.01% πŸ”Ό)
9602/12529
🟑 Branches
71.97% (+0.02% πŸ”Ό)
4751/6601
🟑 Functions 76.56% 2485/3246
🟑 Lines
77.15% (+0.01% πŸ”Ό)
9074/11762

Test suite run success

2271 tests passing in 983 suites.

Report generated by πŸ§ͺjest coverage report action from 8ba7901d9ed92138481c2acb19c5650d84737309

github-actions[bot] avatar Apr 25 '25 08:04 github-actions[bot]

Differences in type declarations

We 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:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/session/exchange.d.ts
@@ -26,33 +26,15 @@ export declare function exchangeAccessForApplicationTokens(identityToken: Identi
  */
 export declare function refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken>;
 /**
- * Given a custom CLI token passed as ENV variable, request a valid Partners API token
+ * Given a custom CLI token passed as ENV variable, request a valid partners API token
  * This token does not accept extra scopes, just the cli one.
- * @param token - The CLI token passed as ENV variable 
+ * @param token - The CLI token passed as ENV variable
  * @returns An instance with the application access tokens.
  */
 export declare function exchangeCustomPartnerToken(token: string): Promise<{
     accessToken: string;
     userId: string;
 }>;
-/**
- * Given a custom CLI token passed as ENV variable, request a valid App Management API token
- * @param token - The CLI token passed as ENV variable 
- * @returns An instance with the application access tokens.
- */
-export declare function exchangeCliTokenForAppManagementAccessToken(token: string): Promise<{
-    accessToken: string;
-    userId: string;
-}>;
-/**
- * Given a custom CLI token passed as ENV variable, request a valid Business Platform API token
- * @param token - The CLI token passed as ENV variable 
- * @returns An instance with the application access tokens.
- */
-export declare function exchangeCliTokenForBusinessPlatformAccessToken(token: string): Promise<{
-    accessToken: string;
-    userId: string;
-}>;
 type IdentityDeviceError = 'authorization_pending' | 'access_denied' | 'expired_token' | 'slow_down' | 'unknown_failure';
 /**
  * Given a deviceCode obtained after starting a device identity flow, request an identity token.

packages/cli-kit/dist/private/node/session/scopes.d.ts
@@ -13,10 +13,4 @@ export declare function allDefaultScopes(extraScopes?: string[]): string[];
  * @param extraScopes - custom user-defined scopes
  * @returns Array of scopes
  */
-export declare function apiScopes(api: API, extraScopes?: string[]): string[];
-/**
- * Returns specific scopes required for token exchange with the given API.
- * @param api - API to get the scopes for
- * @returns Array of transformed scopes
- */
-export declare function tokenExchangeScopes(api: API): string[];
\ No newline at end of file
+export declare function apiScopes(api: API, extraScopes?: string[]): string[];
\ No newline at end of file

packages/cli-kit/dist/public/node/context/local.d.ts
@@ -25,6 +25,13 @@ export declare function isDevelopment(env?: NodeJS.ProcessEnv): boolean;
  * @returns True if SHOPIFY_FLAG_VERBOSE is truthy or the flag --verbose has been passed.
  */
 export declare function isVerbose(env?: NodeJS.ProcessEnv): boolean;
+/**
+ * It returns true if the App Management API is disabled.
+ * This should only be relevant when using a Partners token.
+ *
+ * @returns True if the App Management API is disabled.
+ */
+export declare function isAppManagementDisabled(): boolean;
 /**
  * Returns true if the environment in which the CLI is running is either
  * a local environment (where dev is present) or a cloud environment (spin).

github-actions[bot] avatar Apr 25 '25 09:04 github-actions[bot]

We detected some changes at packages/*/src 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.

github-actions[bot] avatar Apr 25 '25 09:04 github-actions[bot]

It shouldn't fail in either case, for this specific example, the editor collection extension doesn't use metafields for anything so:

  • If you have metafields in your editor_extension_collection toml and metafields IS part of the schema:

    • The metafields info will be parsed, but then the specification won't use it. all OK
  • If you have metafields in your toml and metafields IS NOT part of the schema:

    • That's ok, zod shemas strip out unrecognized keys during parsing, and since it's not needed by the spec, everything will work.

So I think is safe to remove the metafields property from the schemas that don't need it

isaacroldan avatar May 05 '25 11:05 isaacroldan

We detected some changes at packages/*/src 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.

[!CAUTION] DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

github-actions[bot] avatar May 09 '25 09:05 github-actions[bot]

@shauns updated tests to ensure extensions are parsed correctly even when having an extra unknown field in their toml

isaacroldan avatar May 09 '25 09:05 isaacroldan

@shauns let me know if I can merge this one πŸ‘Œ

isaacroldan avatar May 14 '25 13:05 isaacroldan