Shopify / cli

Build apps, themes, and hydrogen storefronts for Shopify
https://shopify.dev
MIT License
436 stars 128 forks source link

[Themes] Fix empty files in host themes during `app dev` #4813

Closed jamesmengo closed 1 week ago

jamesmengo commented 1 week ago

WHY are these changes introduced?

https://github.com/Shopify/cli/issues/4793

When using app dev, host themes were getting overwritten due to a race condition between the minimum asset upload and the core job that handles theme installation when the src param`, causing potential issues during development.

WHAT is this pull request doing?

Prevents the upload of of minimum theme assets when a source URL is provided during theme creation.

How to test your changes?

  1. Run app dev with a theme extension
  2. Verify that the host theme is created with proper content
  3. Ensure no empty files are present in the theme

Measuring impact

Checklist

jamesmengo commented 1 week ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jamesmengo and the rest of your teammates on Graphite Graphite

github-actions[bot] commented 1 week ago

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
72.05% (+0.01% πŸ”Ό)
8354/11595
🟑 Branches
68.49% (+0.03% πŸ”Ό)
4041/5900
🟑 Functions 71.33% 2192/3073
🟑 Lines
72.43% (+0.01% πŸ”Ό)
7900/10907

Test suite run success

1893 tests passing in 867 suites.

Report generated by πŸ§ͺjest coverage report action from 7d89068e323d90d7ca7690b4042ac8c9c66ed069

jamesmengo commented 1 week ago

/snapit

github-actions[bot] commented 1 week ago

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

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20241108034419

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.

jamesmengo commented 1 week ago

Thanks! I think this makes sense regardless of wether it fixes that specific issue or not, right? πŸ€”

yep, and I was able to confirm that this fixes the issue :)