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

Fix shopify theme dev to no longer fail when development themes expire in internationalized stores #5200

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Jan 15, 2025

WHY are these changes introduced?

Fixes #5192

Users were facing the error above because internationalized stores return internationalized error messages in the new GQL API, which were not being handled as expected in the CLI.

WHAT is this pull request doing?

This PR changes the fetchTheme API to no longer abort or throw an error. API consumers are already handling the absence of the theme when the function returns undefined. Some abort the execution, but others create the necessary theme.

I've checked other possible similar scenarios, and I spotted no more occurrences of error messages being handled this way, so this PR should be enough.

How to test your changes?

  • Update the language of your store to "Português (Brasil)"
  • Run dev cd dawn
  • Run shopify theme dev and get your development theme id
  • Update ~/Library/Preferences/shopify-cli-development-theme-config-nodejs/config.json by appending 1 to your development theme id there (now, your development theme will no longer exist)
  • Run dev cd dawn
  • Run shopify theme dev
  • Notice it just creates a new development theme

Before:
Screenshot 2025-01-15 at 13 43 39

After:
Screenshot 2025-01-15 at 13 44 10

Post-release steps

N/A

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

@karreiro karreiro requested review from a team as code owners January 15, 2025 12:48
Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.12% (+0.01% 🔼)
8876/11815
🟡 Branches
70.29% (+0.03% 🔼)
4336/6169
🟡 Functions 75.04% 2324/3097
🟡 Lines
75.69% (+0.03% 🔼)
8392/11088
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%
🟡
... / api.ts
59.28% (-0.02% 🔻)
44.87% (-0.25% 🔻)
60%
60.76% (+0.02% 🔼)

Test suite run success

2005 tests passing in 905 suites.

Report generated by 🧪jest coverage report action from 1a5aec2

Copy link
Contributor

@EvilGenius13 EvilGenius13 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 for the quick fix! 🚀

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.

[Bug]: theme dev: Failed to fetch theme: <non-existent-theme-id>
2 participants