-
Notifications
You must be signed in to change notification settings - Fork 129
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 - Do not display warnings when file deletion fails #4426
Conversation
cb98d0b
to
9e75d39
Compare
Coverage report
Show files with reduced coverage 🔻
Test suite run success1889 tests passing in 857 suites. Report generated by 🧪jest coverage report action from d9691b8 |
.then(async (success) => { | ||
if (!success) throw new Error('Unknown issue.') | ||
outputSyncResult('delete', fileKey) | ||
fetchThemeAsset(Number(themeId), fileKey, adminSession) |
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.
I'm debating whether we should make this call explicitly, or just call deleteThemeAsset
(which will fail) and handle the failures silently.
This would save an additional call every time a file is deleted, but the tradeoff with that approach would mean that we are unable to discern between legitimate failures of deleteThemeAsset
(such as a server failure) from instances where the failure comes from the asset not existing on the remote theme.
Update:
We landed on handling the failures silently rather than adding additional calls to check for file existence on the remote theme
This PR will prevent these calls being made in the first place
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
4e86ef6
to
c89a7fc
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.
Thanks for also adding the test!
const deleteOperationPromise = new Promise<void>((resolve) => { | ||
themeFileSystem.addEventListener('unlink', () => { | ||
setImmediate(resolve) | ||
}) | ||
}) |
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.
FYI there's also vi.waitFor
in Vitest to deal with async code, but I think the promise here is actually better 👍
c89a7fc
to
d9691b8
Compare
WHY are these changes introduced?
Fixes https://github.com/Shopify/develop-advanced-edits/issues/310
WHAT is this pull request doing?
- Prevents the watcher from attempting to delete files that don't exist on the local themeoutputDebug
when the file deletion fails (either due to server error or because the file already doesn't exist)How to test your changes?
Setup:
shopify theme push -d
-> delete files from the code editorshopify-dev theme dev --dev-preview --theme-editor-sync --verbose
verbose
logs. You should see logs like:Failed to delete file "${fileKey}" from remote theme.
BEFORE
AFTER
Visual.Studio.Code.-.Dawn.mp4
Measuring impact
How do we know this change was effective? Please choose one:
Checklist