Fix --nodelete behaviour on dev command #4662
Open
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
EvilGenius13
force-pushed
the
change-nodelete-on-dev
branch
from
11ac5b5
to
689aeac
Compare
Coverage report
Show files with reduced coverage 🔻
Test suite run success1917 tests passing in 871 suites. Report generated by 🧪jest coverage report action from e97fb51 |
EvilGenius13
changed the title
This comment has been minimized.
This comment has been minimized.
EvilGenius13
force-pushed
the
change-nodelete-on-dev
branch
from
689aeac
to
043c81d
Compare
This fixes an issue where the --nodelete flag was not being respected when running the dev command. This was due to the fact that the flag was not being passed to the theme-fs utility function.
EvilGenius13
force-pushed
the
change-nodelete-on-dev
branch
from
043c81d
to
e97fb51
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/node/themes/types.d.ts@@ -28,6 +28,7 @@ export interface ThemeFileSystemOptions {
only?: string[];
};
notify?: string;
+ noDelete?: boolean;
}
/**
* Represents a theme on the file system.
|
graygilmore
approved these changes
Oct 18, 2024
| @@ -178,7 +178,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti | |||
| } | |||
|
|
|||
| const handleFileDelete = (themeId: string, adminSession: AdminSession, fileKey: string) => { | |||
| if (isFileIgnored(fileKey)) return | |||
| if (isFileIgnored(fileKey) || options?.noDelete) return | |||
There was a problem hiding this comment.
Maybe we can add a test case for this?
jamesmengo
reviewed
Oct 18, 2024
| @@ -178,7 +178,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti | |||
| } | |||
|
|
|||
| const handleFileDelete = (themeId: string, adminSession: AdminSession, fileKey: string) => { | |||
| if (isFileIgnored(fileKey)) return | |||
| if (isFileIgnored(fileKey) || options?.noDelete) return | |||
There was a problem hiding this comment.
Can we add a test for this? Maybe for the ignore as well ❤️
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?
Fixes: https://github.com/Shopify/develop-advanced-edits/issues/370
While fixing up some of the documentation relating to using the
--nodeleteflag, I noticed that files were still being deleted remotely when usingtheme dev --nodelete.WHAT is this pull request doing?
The
mountThemeFileSystemwas not receiving the--nodeleteflag if it was being used. I added it to theThemeFileSystemOptionstype and now we check if it present in thehandleFileDeletemethod. If it is, we return early.How to test your changes?
Run the current build and use the command
theme dev --nodeleteMake a file.
Delete the file.
You should see it be deleted remotely.
Pull down this branch
Build this branch
Run
theme dev --nodeleteMake a file
Delete the file
You will see it keeps the remote file.
Measuring impact
How do we know this change was effective? Please choose one:
Checklist