Skip to content

Conversation

lizkenyon
Copy link
Contributor

WHY are these changes introduced?

Fixes #2643

  • We were not properly passing the signal parameter properly through to the request.
  • Our tests for this were falsely passing

WHAT is this pull request doing?

  • Modifies the utilities function in the graphql client to properly pass the signal param
  • Mocking the functionality of of the abort signal proves difficult in our current test setup. (The prior tests were falsely passing.)
  • Modifies the test to just check that the param is properly being passed through.
screencast.2025-07-17.09-33-35.mp4

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used pnpm changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@lizkenyon lizkenyon requested a review from a team as a code owner July 17, 2025 14:41
@byrichardpowell byrichardpowell requested a review from Copilot July 17, 2025 14:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This patch ensures the abort signal option is correctly propagated through GraphQL and REST requests and updates related tests and utilities.

  • Updated GraphQL client utilities and types to handle an optional signal parameter.
  • Introduced mockRequestCapture in the mock adapter to record RequestInit and reset it in tests.
  • Revised tests in multiple packages to assert that the abort signal is passed to fetch or the mock adapter.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/apps/shopify-app-remix/src/server/authenticate/admin/tests/admin-client.test.ts Add test to verify abort signal is passed to fetch
packages/apps/shopify-app-react-router/src/server/authenticate/admin/tests/admin-client.test.ts Add test to verify abort signal propagation
packages/apps/shopify-api/lib/clients/admin/tests/admin_graphql_client.test.ts Update GraphQL client test to capture and assert the abort signal
packages/apps/shopify-api/adapters/mock/adapter.ts Introduce mockRequestCapture to record RequestInit including signal
packages/apps/shopify-api/adapters/mock/mock_test_requests.ts Reset mockRequestCapture in the mock reset method
packages/api-clients/graphql-client/src/api-client-utilities/utilities.ts Extend generateGetGQLClientParams to forward signal option
packages/api-clients/graphql-client/src/api-client-utilities/types.ts Add signal?: AbortSignal to request options type
packages/api-clients/graphql-client/src/api-client-utilities/tests/utilities.test.ts Add unit test for signal in generated GraphQL client params
.changeset/curly-masks-feel.md Record patch entries for signal support
Comments suppressed due to low confidence (3)

packages/api-clients/graphql-client/src/api-client-utilities/tests/utilities.test.ts:147

  • [nitpick] You may also want to add a test that combines signal with other options (e.g., retries, headers, variables) to verify that all options are merged correctly.
    it('returns an array with the operation string and an option with signal when an abort signal was provided', () => {

packages/apps/shopify-app-remix/src/server/authenticate/admin/tests/admin-client.test.ts:183

  • [nitpick] It’s best to reset fetchMock before this test (e.g., in a beforeEach) to ensure no prior calls interfere with the call count assertion.
  it('passes abort signal through to fetch request', async () => {

packages/apps/shopify-app-react-router/src/server/authenticate/admin/tests/admin-client.test.ts:37

  • [nitpick] Consider resetting fetchMock in a beforeEach block here as well to isolate this test and avoid residual mock calls.
  it('passes abort signal through to fetch request', async () => {

headers,
retries,
signal,
} = options as any;
Copy link
Preview

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Avoid using as any here. You can extend the RequestParams type (or the function signature) to include signal so you keep full type safety instead of casting to any.

Copilot uses AI. Check for mistakes.

@lizkenyon lizkenyon force-pushed the liz/fix-passing-abort-signal branch from ee9f210 to ca3c24d Compare July 17, 2025 18:03
@lizkenyon lizkenyon force-pushed the liz/fix-passing-abort-signal branch from ca3c24d to 3100acd Compare July 17, 2025 19:11
@lizkenyon lizkenyon merged commit 70f3380 into main Jul 21, 2025
6 checks passed
@lizkenyon lizkenyon deleted the liz/fix-passing-abort-signal branch July 21, 2025 13:10
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.

Shopify Graphql Api does not leverage AbortSignal as described
2 participants