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

[ShopifyVM] cli use new dev dash endpoints for polling #5443

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mssalemi
Copy link
Contributor

@mssalemi mssalemi commented Feb 20, 2025

Addresses: https://github.com/Shopify/shopify-functions/issues/606

WHY are these changes introduced?

Currently, app logs are fetched directly from Partners API. We need to use the new core endpoints set out in this branch.

Current Implementation:

  1. Subscribe to Logs

    • Direct GraphQL mutation to Partners: https://${partnersFqdn}/api/cli/graphql
    • Uses AppLogsSubscribeMutation to get JWT token
    • Not Implemented in app-management-client.ts
  2. Poll Logs

    • Direct HTTP GET to Partners: https://${partnersFqdn}/app_logs/poll
    • Uses JWT token from subscription
    • Implemented in utils.ts
    • hardcoded to go to partners

WHAT is this pull request doing?

Updated the AppManagementClient to implement the subscribeToAppLogs method.

  • This requires updating developerPlatformClient interface - we need access to apiKey organizationId, appId for request to dev dash (these are not required for partners requests).
  • I updated the interface similarly when updating the targetSchemaDefinition fields
  • This method is previously not implemented and throws and error

Updated PollAppLogs

  • currently I just added the method, but will update this so it uses the correct fetch method based on developer platform client. This might make more sense long term to refactor this and move it to the developer platform client.
  • But for now, I will update this so we just call the code conditionally

How to test your changes?

  • Todo
export USE_APP_MANAGEMENT_API=1
export NODE_TLS_REJECT_UNAUTHORIZED=0
export SHOPIFY_SERVICE_ENV=spin
export SPIN_INSTANCE=$(spin show -o name)

pnpm run shopify app <command>

Post-release steps

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

Copy link
Contributor

Unused exports (1)

Filename exports
packages/app/src/cli/services/app-logs/utils.ts fetchAppLogsDevDashboard

@mssalemi mssalemi force-pushed the ms.proto-dev-dash-app-logs branch from 61beb0d to 923b09f Compare February 20, 2025 16:41
@mssalemi mssalemi force-pushed the ms.proto-dev-dash-app-logs branch from 923b09f to 7b779b7 Compare February 20, 2025 18:13
Copy link
Contributor

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/public/node/api/functions.d.ts
@@ -10,4 +10,23 @@ import { Variables } from 'graphql-request';
  * @param variables - Optional query variables.
  * @returns Promise resolving to the typed query result.
  */
-export declare function functionsRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, appId: string, variables?: TVariables): Promise<TResult>;
\ No newline at end of file
+export declare function functionsRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, appId: string, variables?: TVariables): Promise<TResult>;
+interface AppLogsSubscribeResponse {
+    appLogsSubscribe: {
+        success: boolean;
+        errors?: string[];
+        jwtToken: string;
+    };
+}
+/**
+ * Subscribes to app logs through the Functions API.
+ *
+ * @param orgId - Organization identifier.
+ * @param token - Authentication token.
+ * @param appId - App identifier.
+ * @param shopIds - Array of shop IDs to subscribe to.
+ * @param apiKey - API key for the app.
+ * @returns Promise resolving to the subscription response.
+ */
+export declare function functionsAppLogsSubscribe(orgId: string, token: string, appId: string, shopIds: string[], apiKey: string): Promise<AppLogsSubscribeResponse>;
+export {};
\ No newline at end of file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant