[Themes] Feat: Poll for JSON changes when the themeEditorSync flag is passed to the dev command
#3894
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success1650 tests passing in 766 suites. Report generated by 🧪jest coverage report action from 1a52026 |
|
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
jamesmengo
force-pushed
the
jmeng/theme_sync_poll_clean
branch
from
e6599fd
to
86bfbbd
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/public/common/version.d.ts@@ -1 +1 @@
-export declare const CLI_KIT_VERSION = "3.60.0";
\ No newline at end of file
+export declare const CLI_KIT_VERSION = "3.59.0";
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/TextPrompt.d.ts@@ -11,7 +11,6 @@ export interface TextPromptProps {
emptyDisplayedValue?: string;
abortSignal?: AbortSignal;
preview?: (value: string) => TokenItem<InlineToken>;
- initialAnswer?: string;
}
declare const TextPrompt: FunctionComponent<TextPromptProps>;
export { TextPrompt };
\ No newline at end of file
|
karreiro
reviewed
May 14, 2024
There was a problem hiding this comment.
Thank you for this PR, @jamesmengo! 🚀 Please, let me know what you think about them :)
jamesmengo
force-pushed
the
jmeng/theme_sync_poll_clean
branch
2 times, most recently
from
edc69e5
to
95407dd
Compare
jamesmengo
commented
May 15, 2024
jamesmengo
commented
May 15, 2024
packages/theme/src/cli/utilities/theme-environment/theme-polling.ts
Outdated
Show resolved
Hide resolved
karreiro
added
the
#gsd:40767
jamesmengo
force-pushed
the
jmeng/theme_sync_poll_clean
branch
4 times, most recently
from
f1ba15a
to
77d53ed
Compare
jamesmengo
commented
May 15, 2024
| } | ||
|
|
||
| await uploadTheme(theme, ctx.session, ctx.remoteChecksums, ctx.localThemeFileSystem, {}) |
There was a problem hiding this comment.
Since we are handling theme upload here, I added a comment here to clarify that the remote-theme-watcher does not upload files
| expect(initializeThemeEditorSync).toHaveBeenCalled() | ||
| expect(uploadTheme).not.toHaveBeenCalled() | ||
| expect(reconcileAndPollThemeEditorChanges).toHaveBeenCalled() | ||
| expect(uploadTheme).toHaveBeenCalled() |
There was a problem hiding this comment.
Handling upload in theme-environment now :)
packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts
Outdated
Show resolved
Hide resolved
jamesmengo
changed the title
themeEditorSync flag is passed to the dev command
karreiro
reviewed
May 16, 2024
There was a problem hiding this comment.
Thank you, @jamesmengo! Really expressive code! I've left some minor comments; please let me know what you think about them :)
packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts
Outdated
Show resolved
Hide resolved
packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.test.ts
Outdated
Show resolved
Hide resolved
packages/theme/src/cli/utilities/theme-environment/remote-theme-watcher.ts
Outdated
Show resolved
Hide resolved
jamesmengo
force-pushed
the
jmeng/theme_sync_poll_clean
branch
4 times, most recently
from
e37611e
to
f57c353
Compare
Initiate polling for theme editor changes Update local theme file system after reconciling files Modify theme polling reconciliation strategy Fix: Exit process when local and remote values are both changed Move Theme Upload to Theme-Environment instead of AssetFileSyncer Refactor - file reconciliation strategy to accept callbacks Add user-facing message when initiating asset synchronization and polling Handle AbortError properly when multiple source changes are detected Extract Theme Polling logic to separate file Extract Theme Reconciliation logic to its own file Render warning to users when there is a non-fatal error when polling remote changes Throw all errors when polling remote changes instead of retrying on non-fatal errors Refactor theme reconciliation to handle only JSON files Refactor theme polling to handle only JSON files Rename asset-file-syncer to remote-theme-watcher Remove call to fetchChecksums by updating remote checksums locally Move theme-environment.ts under the theme-environment folder Convert to JSDOC Revert "Remove call to fetchChecksums by updating remote checksums locally" This reverts commit fc5a1fa. Rename test describe block Create new themeFileSystem object for theme polling Render polling error to UI
jamesmengo
force-pushed
the
jmeng/theme_sync_poll_clean
branch
from
f57c353
to
1a52026
Compare
karreiro
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
WHAT is this pull request doing?
How to test your changes?
pnpm shopify theme dev --path=<PATH_TO_THEME> --dev-preview --theme-editor-syncRemote Changes -> should automatically propagate locally
deleting a file in your editor -> should delete locally
modifying a file in your editor -> should update local theme
creating a file in your editor -> should download locally
Local Changes (No-op)
delete/modify/create a file in your local theme -> do nothing (this will be handled by FS Watcher)
Remote + local Changes
delete/modify/create a file in your local theme that is also changed on remote -> Abort the entire process
Code.-.cli.packages_theme_src_cli_utilities_asset-file-syncer.ts.cli.Workspace.mp4
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
devordeployhave been reflected in the internal flowchart.