-
Notifications
You must be signed in to change notification settings - Fork 126
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] Theme Dev - Editor Sync - Fix Hanging Theme Reconciliation Progress Bar and User Inputs #4392
Conversation
fc4a461
to
deafb9b
Compare
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
Coverage report
Test suite run success1862 tests passing in 845 suites. Report generated by 🧪jest coverage report action from 9fb98ce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! I had no idea that --theme-editor-sync
required prompts, I would have done things differently in other PRs if I knew it 😅
Looking at the changes, I think we're going to have a race condition between the Theme Sync prompts and the Password prompt (or any other prompt we add in the future).
Generally speaking, we shouldn't have 2 different background tasks that render things to the terminal. That's why it's a good idea to separate background work from prompts / loading bars.
I think it's hard to fix this issue in the theme editor sync right now, so perhaps easier to change how the password thing works? We'd need to await for its prompt after all the setup is done but before starting the server. I guess we can refactor all of this later and try to find a better flow.
Aside from that, I'm leaving a comment below for different things.
const {reconciliationFinishedPromise, readyForReconciliationPromise} = await reconcileJsonFiles( | ||
targetTheme, | ||
session, | ||
remoteChecksums, | ||
localThemeFileSystem, | ||
options, | ||
) | ||
|
||
const updatedRemoteChecksums = await fetchChecksums(targetTheme.id, session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now async, I don't think updatedRemoteChecksums
is actually getting the updated ones?
Can we make up the updatedRemoteChecksums
variable with the information we already have after the prompts? Merging actual remote checksums + checksums we have locally that we are going to update?
await performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles) | ||
const reconciliationFinishedPromise = readyForReconciliationPromise.then((partitionedFiles) => | ||
performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles), | ||
) | ||
|
||
return { | ||
readyForReconciliationPromise: readyForReconciliationPromise.then(() => {}), | ||
reconciliationFinishedPromise, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to calculate updated remote checksums, perhaps we should keep the prompting here upfront, since there's nothing else we can do without this information.
const partitionedFiles = await partitionFilesByReconciliationStrategy(...)
...
return { workPromise: performFileReconciliation(targetTheme, session, localThemeFileSystem, partitionedFiles) }
|
||
const themeSetupPromise = editorSyncPromise.then( | ||
({updatedRemoteChecksums, reconciliationFinishedPromise, readyForReconciliationPromise}) => { | ||
const uploadPromise = reconciliationFinishedPromise.then(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get the updatedRemoteChecksums
(fabricated locally), I don't think we need to await for reconciliationFinishedPromise
? That work is just downloading files, and we can start uploading in parallel, right? Or am I missing something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! We wait for the download to finish to prevent a race condition between this scenario
-> Download changes on sections/example.json
from remote theme
-> At the same time, we may upload our local version of sections/example.json
if the fileSystem hasn't been updated yet
packages/theme/src/cli/utilities/theme-environment/theme-environment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, @jamesmengo!
We may adopt the following steps to understand the initialization issue we're facing with the reconciliation:
# Open Dawn
$ dev cd dawn
# Let's go to and older version
$ git reset origin/v11.0.0 --hard
# Push that version to our remote development theme
$ shopify theme push -d
# Pull the remote version so we may download the missing "settings" keys that Core adds in some files
$ shopify theme pull -d
# Run:
$ shopify theme dev --dev-preview --theme-editor-sync
We will get these logs:
dawn$ shopify theme dev --dev-preview --theme-editor-sync
This feature is currently in development and is not ready for use or testing yet.
╭─ info ───────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ The files listed below are only present locally. What would you like to do? │
│ • locales/bg.json │
│ • locales/hr.json │
│ • locales/lt.json │
│ • locales/ro.json │
│ • locales/sk.json │
│ • locales/sl.json │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
? Reconciliation Strategy:
✔ Delete files from the local directory
╭─ info ───────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ The files listed below are only present on the remote theme. What would you like to do? │
│ • locales/bg-BG.json │
│ • locales/hr-HR.json │
│ • locales/lt-LT.json │
│ • locales/ro-RO.json │
│ • locales/sk-SK.json │
│ • locales/sl-SI.json │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
? Incorrect password provided. Please try again:
✔ *****
╭─ info ───────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ The files listed below differ between the local and remote versions. What would you like to do? │
│ • config/settings_data.json │
│ • config/settings_schema.json │
│ • locales/cs.json │
│ • locales/cs.schema.json │
│ • locales/da.json │
│ • locales/da.schema.json │
│ • locales/de.json │
│ • locales/de.schema.json │
│ • locales/el.json │
│ • locales/en.default.json │
│ • locales/en.default.schema.json │
│ • locales/es.json │
│ • locales/es.schema.json │
│ • locales/fi.json │
│ • locales/fi.schema.json │
│ • locales/fr.json │
│ • locales/fr.schema.json │
│ • locales/hu.json │
│ • locales/id.json │
│ • locales/it.json │
│ • locales/it.schema.json │
│ • locales/ja.json │
│ • locales/ja.schema.json │
│ • locales/ko.json │
│ • locales/ko.schema.json │
│ • locales/nb.json │
│ • locales/nb.schema.json │
│ • locales/nl.json │
│ • locales/nl.schema.json │
│ • locales/pl.json │
│ • locales/pl.schema.json │
│ • locales/pt-BR.json │
│ • locales/pt-BR.schema.json │
│ • locales/pt-PT.json │
│ • locales/pt-PT.schema.json │
│ • locales/ru.json │
│ • locales/sv.json │
│ • locales/sv.schema.json │
│ • locales/th.json │
│ • locales/th.schema.json │
│ • locales/tr.json │
│ • locales/tr.schema.json │
│ • locales/vi.json │
│ • locales/vi.schema.json │
│ • locales/zh-CN.json │
│ • locales/zh-CN.schema.json │
│ • locales/zh-TW.json │
│ • locales/zh-TW.schema.json │
│ • sections/footer-group.json │
│ • sections/header-group.json │
│ • templates/404.json │
│ • templates/article.json │
│ • templates/cart.json │
│ • templates/collection.json │
│ • templates/customers/account.json │
│ • templates/customers/activate_account.json │
│ • templates/customers/addresses.json │
│ • templates/customers/login.json │
│ • templates/customers/order.json │
│ • templates/customers/register.json │
│ • templates/customers/reset_password.json │
│ • templates/index.json │
│ • templates/page.contact.json │
│ • templates/password.json │
│ • templates/product.json │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
? Incorrect password provided. Please try again:
✔ ******
╭─ success ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Preview your theme │
│ • http://127.0.0.1:9292 │
│ │
│ Next steps │
│ • Customize your theme at the theme editor │
│ • Share your theme preview │
│ (https://cheap-comic-strip-com.myshopify.com/?preview_theme_id=163762274326) │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/ro.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/hr.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/lt.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
• 10:37:06 Synced » download templates/404.json from remote theme
• 10:37:06 Synced » download templates/article.json from remote theme
• 10:37:06 Synced » download templates/cart.json from remote theme
• 10:37:06 Synced » download sections/header-group.json from remote theme
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/bg.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/sl.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ warning ────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to delete file "locales/sk.json". │
│ │
│ Unknown issue. │
│ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
• 10:37:07 Synced » download templates/customers/activate_account.json from remote theme
• 10:37:07 Synced » download templates/customers/account.json from remote theme
• 10:37:07 Synced » download templates/password.json from remote theme
• 10:37:07 Synced » download templates/index.json from remote theme
• 10:37:07 Synced » download templates/product.json from remote theme
• 10:37:11 Synced » download templates/customers/addresses.json from remote theme
• 10:37:11 Synced » download config/settings_schema.json from remote theme
• 10:37:16 Synced » update config/settings_schema.json
• 10:37:17 Synced » download templates/customers/login.json from remote theme
• 10:37:18 Synced » download templates/customers/order.json from remote theme
• 10:37:21 Synced » download templates/customers/register.json from remote theme
• 10:37:21 Synced » download templates/customers/reset_password.json from remote theme
ce8e7ca
to
58e0307
Compare
@@ -53,34 +60,6 @@ describe('reconcileAndPollThemeEditorChanges', async () => { | |||
) | |||
}) | |||
|
|||
test('should not call reconcileJsonFiles when remote theme contains no files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test and logic moved to theme-reconciliation
58e0307
to
9fb98ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo!
Fran is OOO this week, and James has updated the PR accordingly :)
I've raised an issue here that was revealed by this PR but isn't directly related. So, we're proceeding with the merge 🚀 |
WHY are these changes introduced?
WHAT is this pull request doing?
theme reconciliation
progress bar backstore-password
input which prevented users from selecting areconciliation option
How to test your changes?
shopify theme push -d
.This will trigger a job on core which adds
settings
keys in yourJSON
assetsshopify-dev theme dev --dev-preview --theme-editor-sync --store-password=<INCORRECT_PASSWORD>
.You should see a large number of JSON files listed.
Measuring impact
How do we know this change was effective? Please choose one:
Checklist