[Themes] Theme Dev - Editor Sync - Fix Hanging Theme Reconciliation Progress Bar and User Inputs #4392
Conversation
jamesmengo
added
the
#gsd:40767
jamesmengo
force-pushed
the
jm/editorsyncprogress
branch
from
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.
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.
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.
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.
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.
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.
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-syncWe 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
jamesmengo
force-pushed
the
jm/editorsyncprogress
branch
3 times, most recently
from
ce8e7ca
to
58e0307
Compare
jamesmengo
changed the title
| @@ -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.
test and logic moved to theme-reconciliation
jamesmengo
force-pushed
the
jm/editorsyncprogress
branch
from
58e0307
to
9fb98ce
Compare
karreiro
dismissed
frandiox’s stale review
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 reconciliationprogress bar backstore-passwordinput which prevented users from selecting areconciliation optionHow to test your changes?
shopify theme push -d.This will trigger a job on core which adds
settingskeys in yourJSONassetsshopify-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