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] Skip UID update when in unsafe report mode #5171

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

amcaplan
Copy link
Contributor

@amcaplan amcaplan commented Jan 8, 2025

WHY are these changes introduced?

When we run app info on an app with an invalid extension TOML:

  1. The TOML loading fails, but is in unsafe report mode so execution can continue
  2. We replace the config with a default empty object here
  3. Our code thinks there's no UID
  4. It inserts a second UID into the file, making it invalid 🤦

WHAT is this pull request doing?

Skips the UID update when in unsafe report mode, as:

  1. We haven't built sufficient confidence that it is correct to update
  2. We probably shouldn't edit files on app info anyway!

How to test your changes?

  1. Create an app with an extension and change a type (e.g. string to number) in the shopify.extension.toml file
  2. Run shopify app info twice.

On the second time (on main), it will fail with a double UID error. But on this branch, if you start with a clean file, you can run it many times and not hit this issue.

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

@amcaplan amcaplan requested a review from a team as a code owner January 8, 2025 13:06
Copy link
Contributor

github-actions bot commented Jan 8, 2025

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

github-actions bot commented Jan 8, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.15% (+0% 🔼)
8868/11801
🟡 Branches
70.33% (+0.01% 🔼)
4310/6128
🟡 Functions 75.06% 2318/3088
🟡 Lines
75.72% (+0.01% 🔼)
8384/11073
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%

Test suite run success

2000 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 33999f3

@amcaplan amcaplan added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit ed55fc7 Jan 8, 2025
27 checks passed
@amcaplan amcaplan deleted the skip-uid-update-on-report-mode branch January 8, 2025 14:08
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.

2 participants