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

[Bug Fix] Reduce theme upload batch size to prevent timeout #5113

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Dec 16, 2024

WHY are these changes introduced?

Part of #5031

WHAT is this pull request doing?

A previous PR changed the endpoint to upload the themes files from REST API to GraphQL API

  • The new endpoint did not cause the issue, but the PR also increased the theme upload batch size
  • This is generally fine when using the CLI through regular user login, but fails when requests are being proxied through Theme Access App

How to test your changes?

  • Instead Theme Access App on your store
  • Set environment variables in terminal
export SHOPIFY_CLI_THEME_TOKEN=<password from app>
export SHOPIFY_FLAG_STORE=<store>
export SHOPIFY_FLAG_FORCE="1"
  • Run this CLI branch against an existing theme
    • npx shopify theme push --theme="bug-dec16" --unpublished --verbose --path=<path-to-repo>
  • theme should be uploaded

NOTE: Also try uploading a LOT of asset files AND with large asset files (e.g. download large images in asset folder)

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

@aswamy aswamy requested a review from catlee December 16, 2024 13:34
@aswamy aswamy requested review from a team as code owners December 16, 2024 13:34
Copy link
Contributor

github-actions bot commented Dec 16, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.26% (+0.02% 🔼)
8802/11696
🟡 Branches
70.58% (+0.04% 🔼)
4288/6075
🟡 Functions
75% (-0.02% 🔻)
2301/3068
🟡 Lines
75.81% (+0.03% 🔼)
8323/10979
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / context.ts
70.75%
60% (-2.07% 🔻)
73.91% 73.33%
🔴
... / extension.ts
53.85% (-1.71% 🔻)
58.33% (+0.64% 🔼)
50%
54.9% (-1.7% 🔻)
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% 90.48% 98.61%
🟢
... / dev-session.ts
85.96% (-1.03% 🔻)
68.33% (+1.09% 🔼)
91.67% (-0.93% 🔻)
92.93% (-0.4% 🔻)
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
100%
98.33% (-1.67% 🔻)

Test suite run success

1989 tests passing in 898 suites.

Report generated by 🧪jest coverage report action from df9d347

@aswamy aswamy requested a review from graygilmore December 16, 2024 16:38
@Waynenabors
Copy link

Just to be clear, does this address the theme pull issue from #5094? I'm currently having issues with push and pull.

@graygilmore
Copy link
Contributor

@Waynenabors I wouldn't say with certainty, no. The user that says it's a dupe is not a part of Shopify and may have been guessing.

@jamesmengo
Copy link
Contributor

Could we only change these limits when using the theme access app?

Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

Thanks for debugging it Alok! Let's ship this for now until we can properly raise the limits 🚀

Comment on lines +21 to +31
export const MAX_BATCH_FILE_COUNT = 20
// 1MB
export const MAX_BATCH_BYTESIZE = 1024 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment here specifying where is the limit coming from? Or perhaps linking this very PR so the context is not lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I will add a comment explaining why we had to lower this limit.

@aswamy aswamy force-pushed the bugfix/decrease-theme-upload-batch-size branch from 01cb5d6 to df9d347 Compare December 17, 2024 12:43
@Waynenabors
Copy link

Any idea when this might get in? Our deploys are failing and I'm trying to decide if it's worth finding an alternative. Thanks everyone!

@jamesmengo jamesmengo added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit ae29b61 Dec 18, 2024
27 checks passed
@jamesmengo jamesmengo deleted the bugfix/decrease-theme-upload-batch-size branch December 18, 2024 16:25
@craigmichaelmartin
Copy link
Contributor

Any idea when this might get in?

Hey @Waynenabors! This is currently released in @shopify/cli@nightly and will be released in a patched version 3.72.2 for @shopify/cli@latest tomorrow.

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.

7 participants