Skip to content

Conversation

@jordanverasamy
Copy link
Contributor

@jordanverasamy jordanverasamy commented Nov 22, 2025

WHY are these changes introduced?

https://github.com/shop/issues-api-foundations/issues/1096

WHAT is this pull request doing?

Adds behaviour to shopify app execute to match the designs:

  • if --watch and --output-file are both provided, write results to that file after completion
  • if --watch is provided but --output-file is not, write results to STDOUT after completion
  • if --watch is not provided, no change to behaviour

How to test your changes?

Try running shopify app execute with --watch and --output-file and verify it writes to the correct file:

pnpm shopify app execute --path=<PATH_TO_YOUR_APP> --query="{ products { edges { node { id } } } }" --watch --output-file=my-results.jsonl
image image

Then try doing the same thing without --output-file, and verify it prints to STDOUT:

pnpm shopify app execute --path=<PATH_TO_YOUR_APP> --query="{ products { edges { node { id } } } }" --watch
image

Copy link
Contributor Author

jordanverasamy commented Nov 22, 2025

@jordanverasamy jordanverasamy changed the title introduce --output-file flag Write bulk operation results to STDOUT or a file, determined by new --output-file flag Nov 22, 2025
@jordanverasamy jordanverasamy changed the title Write bulk operation results to STDOUT or a file, determined by new --output-file flag Introduce --output-file flag, write bulk operation results to STDOUT or a file Nov 22, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 22, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
79.27% (-0.01% 🔻)
13723/17311
🟡 Branches
73.18% (-0.02% 🔻)
6698/9153
🟡 Functions
79.42% (+0% 🔼)
3530/4445
🟡 Lines
79.64% (-0.01% 🔻)
12968/16284
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / download-bulk-operation-results.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / execute-bulk-operation.ts
92.31% (-5.61% 🔻)
82.35% (-8.27% 🔻)
100%
92% (-5.83% 🔻)

Test suite run success

3406 tests passing in 1386 suites.

Report generated by 🧪jest coverage report action from 4b2da57

@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch 2 times, most recently from cb7b47a to 91d94fd Compare November 22, 2025 00:40
@jordanverasamy jordanverasamy self-assigned this Nov 22, 2025
@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from 91d94fd to 9ae6397 Compare November 22, 2025 00:44
@jordanverasamy jordanverasamy force-pushed the jtv/watch-live-progress branch from c9b42e0 to 5270287 Compare November 22, 2025 00:51
@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from 9ae6397 to 535eb18 Compare November 22, 2025 00:51
@jordanverasamy jordanverasamy changed the base branch from jtv/watch-live-progress to graphite-base/6656 November 24, 2025 20:17
@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from 535eb18 to de334e2 Compare November 24, 2025 20:17
@jordanverasamy jordanverasamy changed the base branch from graphite-base/6656 to jtv/watch-live-progress November 24, 2025 20:17
Comment on lines +4 to +12
export async function downloadBulkOperationResults(url: string): Promise<string> {
const response = await fetch(url)

if (!response.ok) {
throw new AbortError(`Failed to download bulk operation results: ${response.statusText}`)
}

return response.text()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have this function in a different file? is just wrapping a call to fetch​ and a error? And is only used in a single place... best to put the call to fetch directly where it is needed. I'd say the tests don't add much value either (you are just testing an if​)

Copy link
Contributor Author

@jordanverasamy jordanverasamy Nov 25, 2025

Choose a reason for hiding this comment

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

Yeah good question! It's really small and like you said, doesnt have any complexity to test. I waffled on whether it should be in its own file, and ultimately decided to, because we're going to want the same behaviour in the bulk status command (more info here) so may as well have it available to reuse.

I'm happy to just duplicate the function in both execute and status if you would prefer, since it's so simple, just a couple lines.

@jordanverasamy jordanverasamy changed the base branch from jtv/watch-live-progress to graphite-base/6656 November 25, 2025 22:07
@gonzaloriestra
Copy link
Contributor

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

}),
'output-file': Flags.string({
description: 'The file path where results should be written. If not specified, results will be written to STDOUT.',
env: 'SHOPIFY_FLAG_OUTPUT_FILE',
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, if you add this it will throw an error when used without watch

Suggested change
env: 'SHOPIFY_FLAG_OUTPUT_FILE',
env: 'SHOPIFY_FLAG_OUTPUT_FILE',
compatible: ['watch'],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I tried this and it didn't work for me:

pnpm shopify app execute --path="../my-second-test-app" --query="{ products { edges { node { id variants { edges { node { id } } } } } } }" --output-file='hello.jsonl'
image

but maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know and I'm happy to add this in a followup though :)

@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from de334e2 to 57c73ad Compare November 26, 2025 18:35
@jordanverasamy jordanverasamy changed the base branch from graphite-base/6656 to jtv/watch-live-progress November 26, 2025 18:36
@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from 57c73ad to 989db7c Compare November 26, 2025 18:51
@jordanverasamy jordanverasamy force-pushed the jtv/watch-live-progress branch from c8e62d6 to 4c19852 Compare November 26, 2025 18:51
@jordanverasamy jordanverasamy marked this pull request as ready for review November 26, 2025 18:53
@jordanverasamy jordanverasamy requested review from a team as code owners November 26, 2025 18:53
@github-actions
Copy link
Contributor

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.

@jordanverasamy jordanverasamy force-pushed the jtv/add-output-file-support branch from 989db7c to 4b2da57 Compare November 26, 2025 19:16
Base automatically changed from jtv/watch-live-progress to main November 26, 2025 19:33
@jordanverasamy jordanverasamy added this pull request to the merge queue Nov 26, 2025
Merged via the queue into main with commit f00e623 Nov 26, 2025
23 checks passed
@jordanverasamy jordanverasamy deleted the jtv/add-output-file-support branch November 26, 2025 21:14
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.

4 participants