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] Add debug logging for theme fetch errors #5203

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Jan 15, 2025

WHY are these changes introduced?

Adds debug logging to report that themeFetch failed on top of the changes introduced in #5200

This is likely inconsequential, but may come in handy for debugging something in the future (even more so because the client will handle failures silently otherwise)

@karreiro Do we want to do this? Would we be adding too much noise?

Copy link
Contributor Author

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

Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.2% (+0.01% 🔼)
8893/11826
🟡 Branches
70.32% (+0.03% 🔼)
4343/6176
🟡 Functions 75.08% 2326/3098
🟡 Lines
75.75% (+0.02% 🔼)
8408/11099
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% 90.48% 98.61%

Test suite run success

2007 tests passing in 906 suites.

Report generated by 🧪jest coverage report action from 25f9861

@jamesmengo jamesmengo added the Area: @shopify/theme @shopify/theme package issues label Jan 15, 2025 — with Graphite App
@jamesmengo jamesmengo marked this pull request as ready for review January 15, 2025 17:08
@jamesmengo jamesmengo requested review from a team as code owners January 15, 2025 17:08
Copy link
Contributor

We detected some changes at packages/*/src 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.

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 had the same internal debate about this. My initial thought was that the command with the request ID would be enough to provide us some context. However, I don't think this line is harmful :)

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.

I'm +1 for adding this in. Adding a one liner to debug logs wouldn't be too noisy imo.

@jamesmengo jamesmengo added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 3c10c59 Jan 15, 2025
28 of 29 checks passed
@jamesmengo jamesmengo deleted the jm/01-15-add_debug_log_to_fetchtheme_errorhandler branch January 15, 2025 18:26
jamesmengo added a commit that referenced this pull request Jan 15, 2025
…theme_errorhandler

[Themes] Add debug logging for theme fetch errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/theme @shopify/theme package issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants