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: conf store not saving some values on windows #1297

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

isaacroldan
Copy link
Contributor

WHY are these changes introduced?

Paths are weird. At some points in the CLI execution, when working on windows, a directory path might use / or \.
Since we use the the directory as the key to store local information about the app, this led to inconsistencies reading the cached values.

WHAT is this pull request doing?

  • Normalize the directory path before getting/setting values

How to test your changes?

  • yarn dev caches any selected value and a 2nd run should load those values.
  • yarn run info should read the same values

Post-release steps

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
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

@github-actions

This comment has been minimized.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Don't forget the changeset entry

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Benchmark report

The following table contains a summary of the startup time for all commands.

Status Command Baseline (avg) Current (avg) Diff
🟢 app build 878.67 ms 887 ms 0.95 %
🟢 app deploy 1073.67 ms 1071.67 ms -0.19 %
🟢 app dev 1068 ms 1087.67 ms 1.84 %
🟢 app env pull 974.67 ms 985 ms 1.06 %
🟢 app env show 973.33 ms 979.67 ms 0.65 %
🟢 app generate extension 1038.67 ms 1042.67 ms 0.39 %
🟢 app generate schema 1002.33 ms 1007.67 ms 0.53 %
🟢 app info 977 ms 994.33 ms 1.77 %
🟢 app scaffold extension 1041.67 ms 1037.67 ms -0.38 %
🟢 app update-url 956.33 ms 970 ms 1.43 %
🟢 theme check 830 ms 833.67 ms 0.44 %
🟢 theme delete 947 ms 958.33 ms 1.2 %
🟢 theme dev 949.33 ms 949.67 ms 0.04 %
🟢 theme help-old 830.33 ms 835.67 ms 0.64 %
🟢 theme info 878 ms 881.33 ms 0.38 %
🟢 theme init 851.67 ms 855.67 ms 0.47 %
🟢 theme language-server 831 ms 837.67 ms 0.8 %
🟢 theme list 946 ms 939 ms -0.74 %
🟢 theme open 959.33 ms 942 ms -1.81 %
🟢 theme package 878.67 ms 874.33 ms -0.49 %
🟢 theme publish 934.33 ms 936.67 ms 0.25 %
🟢 theme pull 939.33 ms 935.33 ms -0.43 %
🟢 theme push 935.67 ms 940.67 ms 0.53 %
🟢 theme serve 945.67 ms 946 ms 0.04 %
🟢 theme share 929 ms 934.33 ms 0.57 %
🟢 webhook trigger 940 ms 943 ms 0.32 %

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
71.81% (+0.02% 🔼)
3943/5491
🟡 Branches 69.23% 1782/2574
🟡 Functions 70.22% 1035/1474
🟡 Lines
73.05% (+0.02% 🔼)
3762/5150

Test suite run success

998 tests passing in 518 suites.

Report generated by 🧪jest coverage report action from 3314a97

@isaacroldan isaacroldan merged commit 50c3606 into main Feb 6, 2023
@isaacroldan isaacroldan deleted the fix-app-conf-store branch February 6, 2023 15:31
@shopify-shipit shopify-shipit bot temporarily deployed to production February 6, 2023 17:35 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to nightly February 7, 2023 02:14 Inactive
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