[Themes] - Theme Dev - Reduce API calls and IO operations triggered from theme reconciliation step

[jamesmengo]

WHY are these changes introduced?

Fixes https://github.com/Shopify/develop-advanced-edits/issues/330

While this PR prevents errors from being surfaced when we delete a local file during the reconciliation step, we're still making a call for each file there to check for its existence. These changes prevent us from making those requests in the first place.

Visual.Studio.Code.-.Git_.Staged.Changes.0.files.Dawn.2.mp4

BENEFITS

• prevents us from calling API for local filesystem changes that come from the reconciliation step
• we could potentially remove the call to check that a file exists, reducing the number of API calls for each time a file is deleted from two (one to check if the file exists and one to delete it) to one. However, this needs further consideration.

TRADEOFFS

• if you modify the file system during the prompts (e.g., manually deleting a file), it won't be caught in the change set

WHAT is this pull request doing?

Defers initialization of the theme watcher until after the reconciliation step has completed. This will prevent the service from trying to synchronize files changes that are coming from the reconciliation step, saving an additional API call each time.

How to test your changes?

Example:
-> Create a development theme shopify theme delete -d -f && shopify theme push -d
-> Create a file that will only be present on your local theme
-> Initialize command shopify theme dev --dev-preview --theme-editor-sync --verboes
-> Select delete files from local theme
-> Note that we are no longer outputting a verbose log with: File does not exist on remote theme.

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

[github-actions]

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

• Themes: @shopify/advanced-edits
• UI extensions: @shopify/ui-extensions-cli
  • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
• Hydrogen: @shopify/hydrogen
• Other: @shopify/app-inner-loop

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	73.01% (+0.02% 🔼)	8390/11491
🟡	Branches	69.58% (+0.03% 🔼)	4092/5881
🟡	Functions	71.75%	2179/3037
🟡	Lines	73.36% (+0.02% 🔼)	7939/10822

Test suite run success

1889 tests passing in 857 suites.

Report generated by 🧪jest coverage report action from 60f895a

[jamesmengo]

There are currently 3 scenarios in which the theme reconciliation step may modify the local filesystem.
Below are how they are currently handled, and how they would be impacted with these changes.

Case	Current	With This Change
Delete file only present on local theme	Call API to check if file exists on remote theme, stop because it doesn't.	Save an API call to delete the file on the remote theme for each changed file
Download file only present on remote theme	Handled here. Read updated checksums and choose not to trigger an upload	Save an IO call to read for each changed file
Update file using remote theme value	Handled here. Read updated checksums and choose not to trigger an upload	Save an IO call to read for each changed file

*Read updated checksums and choose not to trigger an upload -> Changes that come from the remote theme will have matching checksums before and after we call read, because we update the remote checksum when we write

[github-actions]

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** 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.

[frandiox]

Thanks! I think it makes sense to delay the watcher start until the reconciliation is over 👍

⚠️ There's just one thing missing: can you replace this line with files.set(asset.key, buildThemeAsset(asset))? That will ensure we get stats in the file when writing it from remote (we were getting it from the watcher's read before).

---

we could potentially remove the call to check that a file exists, reducing the number of API calls for each time a file is deleted from two (one to check if the file exists and one to delete it) to one. However, this needs further consideration.

Yeah, can't we just make the delete call and ignore the "file not found" error? What would be the downside? 🤔

[jamesmengo]

Yeah, can't we just make the delete call and ignore the "file not found" error? What would be the downside? 🤔

Yep! Reached alignment on this PR to simply ignore the file not found error rather than firing an additional call to check for file existence

I believe one of the drawbacks here would be that we can't distinguish between a legitimate failure to delete a file from a failure that arises because the file is already not present.

This would make it harder to introduce something like retries, but since development theme are meant to be ephemeral, I think that's acceptable. Wouldn't be hard to change in the future if we change our mind.