Add theme dev reconciliation strategy flag#7634
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
bc8df22 to
c635c86
Compare
b73773b to
8f6ca1c
Compare
17976d9 to
a727f50
Compare
8f6ca1c to
fee06e6
Compare
cf17902 to
f1bd642
Compare
fee06e6 to
ad55535
Compare
f1bd642 to
e4069c5
Compare
ad55535 to
8d70823
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/private/node/constants.d.ts@@ -7,6 +7,7 @@ export declare const environmentVariables: {
enableCliRedirect: string;
env: string;
firstPartyDev: string;
+ hostedApps: string;
noAnalytics: string;
optOutInstrumentation: string;
appAutomationToken: string;
packages/cli-kit/dist/public/common/version.d.ts@@ -1 +1 @@
-export declare const CLI_KIT_VERSION = "4.1.0";
\ No newline at end of file
+export declare const CLI_KIT_VERSION = "4.0.0";
\ No newline at end of file
packages/cli-kit/dist/public/node/context/local.d.ts@@ -42,6 +42,13 @@ export declare function isShopify(env?: NodeJS.ProcessEnv): Promise<boolean>;
* @returns True if the SHOPIFY_UNIT_TEST environment variable is truthy.
*/
export declare function isUnitTest(env?: NodeJS.ProcessEnv): boolean;
+/**
+ * Returns true if the CLI is running in hosted apps mode.
+ *
+ * @param env - The environment variables from the environment of the current process.
+ * @returns True if the HOSTED_APPS environment variable is truthy.
+ */
+export declare function isHostedAppsMode(env?: NodeJS.ProcessEnv): boolean;
/**
* Returns true if reporting analytics is enabled.
*
|
e4069c5 to
c5d2d3f
Compare
c5d2d3f to
2b45cf7
Compare
1202d52 to
c22f504
Compare
2b45cf7 to
3fcd3c1
Compare
c22f504 to
155a4b3
Compare
3fcd3c1 to
e4ccca0
Compare
155a4b3 to
bc9aaca
Compare
e4ccca0 to
469f7c2
Compare
bc9aaca to
b1b95a3
Compare
1e5307e to
5a86eaf
Compare
2237293 to
9471955
Compare
9471955 to
8d194da
Compare
5a86eaf to
348790c
Compare
8d194da to
d849219
Compare
348790c to
0b212e4
Compare
c82c565 to
aa85d78
Compare
1fa107f to
69e1254
Compare
aa85d78 to
5f73f48
Compare
69e1254 to
bd3c62b
Compare
02fc84c to
278d226
Compare
bd3c62b to
1e98f09
Compare
lucyxiang
left a comment
There was a problem hiding this comment.
Review assisted by pair-review
| env: 'SHOPIFY_FLAG_STANDARD_EVENTS_INSPECTOR', | ||
| default: false, | ||
| }), | ||
| 'reconciliation-strategy': Flags.string({ |
There was a problem hiding this comment.
theme dev --reconciliation-strategy keep-local on its own does nothing and emits no warning, shall we add dependency on theme-editor-sync?
Suggestion: Consider declaring the dependency so oclif surfaces an error instead of silently ignoring the flag:
| 'reconciliation-strategy': Flags.string({ | |
| 'reconciliation-strategy': Flags.string({ | |
| description: | |
| 'How to resolve JSON conflicts when --theme-editor-sync is enabled. Use keep-local to keep local files, keep-remote to keep remote files, or abort to fail instead of prompting.', | |
| options: ['keep-local', 'keep-remote', 'abort'], | |
| env: 'SHOPIFY_FLAG_RECONCILIATION_STRATEGY', | |
| dependsOn: ['theme-editor-sync'], | |
| }), |
| }) | ||
|
|
||
| describe('files only present on remote developer theme', () => { | ||
| test('should download assets from remote theme without prompting when keep-remote strategy is selected', async () => { |
There was a problem hiding this comment.
Can we check that local files do get deleted
| }) | ||
|
|
||
| describe('files with conflicting checksums', () => { | ||
| test('should keep local files without prompting when keep-local strategy is selected', async () => { |
There was a problem hiding this comment.
Similarly check than remote files are deleted
| case 'abort': | ||
| throw new AbortError( | ||
| 'Theme JSON files need reconciliation.', | ||
| `${title}\n${files.map((file) => `- ${file.key}`).join('\n')}`, | ||
| ) |
There was a problem hiding this comment.
How about collecting all files and throwing one AbortError that lists every file needing reconciliation, rather than throwing inside the per-category callback on the first hit.

WHY are these changes introduced?
Fixes https://github.com/shop/issues-develop/issues/22873
The interactivity audit found that theme dev with --theme-editor-sync could still prompt when JSON files needed reconciliation.
WHAT is this pull request doing?
How to test your changes?
Make a change in a theme file (like templates/404.json) locally and remotely and run:
Checklist