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

[Themes] Verify existence of required theme files before initializing storefront session #4810

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Nov 7, 2024

WHY are these changes introduced?

Adds validation to ensure required theme files exist before initializing the dev server session. This helps catch issues with incomplete or corrupted themes that could cause the dev server to fail when initializing the preview session.

WHAT is this pull request doing?

  • Adds a new verifyRequiredFilesExist function that checks for essential theme files (layout/theme.liquid and config/settings_schema.json)

How to test your changes?

  1. Create a theme missing either layout/theme.liquid or config/settings_schema.json on your shop (more instructions below if needed)
  2. Run the theme dev with the theme flag, and specify the --theme flag to point at the theme created above. Verify that it fails with appropriate error message

image.png

Creating Theme with Missing Assets
  • Cherrypick this change on top of the PR branch
  • p build
  • Delete your development theme shopify-dev theme delete -d
  • Run shopify-dev theme dev

The above command should create a new development theme with missing files - you should then see the error being rendered.

Measuring impact

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

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

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

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.04% (+0% 🔼)
8348/11588
🟡 Branches
68.48% (+0.01% 🔼)
4036/5894
🟡 Functions
71.32% (-0.01% 🔻)
2191/3072
🟡 Lines
72.42% (-0% 🔻)
7894/10901

Test suite run success

1890 tests passing in 865 suites.

Report generated by 🧪jest coverage report action from 2e91ff9

@jamesmengo jamesmengo marked this pull request as ready for review November 7, 2024 19:27
@jamesmengo jamesmengo requested review from a team as code owners November 7, 2024 19:27

This comment has been minimized.

Copy link
Contributor Author

/snapit

Copy link
Contributor

github-actions bot commented Nov 7, 2024

🫰✨ 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-20241107193203

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 jamesmengo changed the title verify required theme files before dev server initialization [Themes] Verify required theme files exist before initializing storefront session Nov 7, 2024
@jamesmengo jamesmengo marked this pull request as draft November 7, 2024 19:55
@jamesmengo jamesmengo added the #gsd:40767 Fortify local development experience for Liquid themes label Nov 7, 2024
@jamesmengo jamesmengo force-pushed the jm/11-07-verify_required_theme_files_before_dev_server_initialization branch from ffd46f3 to 7dd5b3f Compare November 7, 2024 20:20
@jamesmengo jamesmengo marked this pull request as ready for review November 7, 2024 20:25
@jamesmengo jamesmengo changed the title [Themes] Verify required theme files exist before initializing storefront session [Themes] Verify existence of required theme files before initializing storefront session Nov 8, 2024
@jamesmengo jamesmengo marked this pull request as draft November 8, 2024 00:50
Copy link
Contributor Author

pivot: only show this when essential is missing to better determine the cause

@jamesmengo jamesmengo marked this pull request as ready for review November 8, 2024 03:15
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.

Looks good, hoping we can get more insights about the missing cookies with this enhanced error 🙌

Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

Thank you, @jamesmengo! I didn't have the chance to test this one, but these changes LGTM :)

@jamesmengo jamesmengo added this pull request to the merge queue Nov 8, 2024
Merged via the queue into main with commit 3114f58 Nov 8, 2024
28 checks passed
@jamesmengo jamesmengo deleted the jm/11-07-verify_required_theme_files_before_dev_server_initialization branch November 8, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:40767 Fortify local development experience for Liquid themes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants