Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix --nodelete behaviour on dev command #4662

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+8 −1
Open

Fix --nodelete behaviour on dev command #4662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/strong-guests-worry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/cli': patch
---

Fix theme dev command deleting remote files even if using --nodelete flag
1 change: 1 addition & 0 deletions packages/cli-kit/src/public/node/themes/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export type ThemeFSEventPayload<T extends ThemeFSEventName = 'add'> = (ThemeFSEv
export interface ThemeFileSystemOptions {
filters?: {ignore?: string[]; only?: string[]}
notify?: string
noDelete?: boolean
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/theme/src/cli/services/dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,17 @@
const localThemeFileSystem = mountThemeFileSystem(options.directory, {
filters: options,
notify: options.notify,
noDelete: options.noDelete,
})

const host = options.host || DEFAULT_HOST

Check warning on line 75 in packages/theme/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/services/dev.ts#L75 

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
if (options.port && !(await checkPortAvailability(Number(options.port)))) {
throw new AbortError(
`Port ${options.port} is not available. Try a different port or remove the --port flag to use an available port.`,
)
}

const port = options.port || String(await getAvailableTCPPort(Number(DEFAULT_PORT)))

Check warning on line 82 in packages/theme/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/services/dev.ts#L82 

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

const storefrontPassword = await storefrontPasswordPromise
const session = await initializeDevServerSession(
Expand Down Expand Up @@ -119,7 +120,7 @@

renderLinks(options.store, String(options.theme.id), host, port)
if (options.open) {
openURL(`http://${host}:${port}`).catch((error: Error) => {

Check warning on line 123 in packages/theme/src/cli/services/dev.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/services/dev.ts#L123 

[@typescript-eslint/use-unknown-in-catch-callback-variable] Prefer the safe `: unknown` for a catch callback variable.
renderWarning({headline: 'Failed to open the development server.', body: error.stack ?? error.message})
})
}
Expand Down
2 changes: 1 addition & 1 deletion packages/theme/src/cli/utilities/theme-fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,12 @@
switch (eventName) {
case 'add':
case 'change':
return handleFileUpdate(eventName, themeId, adminSession, fileKey)

Check warning on line 103 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L103 

[@typescript-eslint/no-confusing-void-expression] Returning a void expression from a function is forbidden. Please move it before the `return` statement.
case 'unlink':
return handleFileDelete(themeId, adminSession, fileKey)

Check warning on line 105 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L105 

[@typescript-eslint/no-confusing-void-expression] Returning a void expression from a function is forbidden. Please move it before the `return` statement.
}
})
.catch((error) => {

Check warning on line 108 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L108 

[@typescript-eslint/use-unknown-in-catch-callback-variable] Prefer the safe `: unknown` for a catch callback variable.
outputWarn(`Error handling file event for ${fileKey}: ${error}`)
})
}
Expand Down Expand Up @@ -133,7 +133,7 @@
unsyncedFileKeys.add(fileKey)
}

return file.value || file.attachment || ''

Check warning on line 136 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L136 

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

Check warning on line 136 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L136 

[@typescript-eslint/prefer-nullish-coalescing] Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.
})

const syncPromise = contentPromise
Expand All @@ -155,7 +155,7 @@

return true
})
.catch(createSyncingCatchError(fileKey, 'upload'))

Check warning on line 158 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L158 

[@typescript-eslint/use-unknown-in-catch-callback-variable] Prefer the safe `: unknown` for a catch callback variable.

emitEvent(eventName, {
fileKey,
Expand All @@ -178,7 +178,7 @@
}

const handleFileDelete = (themeId: string, adminSession: AdminSession, fileKey: string) => {
if (isFileIgnored(fileKey)) return
if (isFileIgnored(fileKey) || options?.noDelete) return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a test case for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this? Maybe for the ignore as well ❤️


// Optimistically delete the file from the local file system.
files.delete(fileKey)
Expand All @@ -197,7 +197,7 @@
outputSyncResult('delete', fileKey)
return true
})
.catch((error) => {

Check warning on line 200 in packages/theme/src/cli/utilities/theme-fs.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/theme/src/cli/utilities/theme-fs.ts#L200 

[@typescript-eslint/use-unknown-in-catch-callback-variable] Prefer the safe `: unknown` for a catch callback variable.
createSyncingCatchError(fileKey, 'delete')(error)
return false
})
Expand Down
Loading