Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ vi.mock('@shopify/cli-kit/node/system', async () => {
vi.mock('../../../context.js')
vi.mock('../../fetch.js')
vi.mock('../../processes/dev-session.js')
vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => {
const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun')
return {
...actual,
waitForPostRunHookAndExit: vi.fn(),
}
})

const developerPlatformClient = testDeveloperPlatformClient()

Expand Down
10 changes: 2 additions & 8 deletions packages/app/src/cli/services/dev/ui/components/Dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink'
import {handleCtrlC} from '@shopify/cli-kit/node/ui'
import {openURL} from '@shopify/cli-kit/node/system'
import figures from '@shopify/cli-kit/node/figures'
import {isUnitTest} from '@shopify/cli-kit/node/context/local'
import {treeKill} from '@shopify/cli-kit/node/tree-kill'
import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun'
import {Writable} from 'stream'

export interface DeveloperPreviewController {
Expand Down Expand Up @@ -74,12 +73,7 @@ const Dev: FunctionComponent<DevProps> = ({
setIsShuttingDownMessage('Shutting down dev because of an error ...')
} else {
setIsShuttingDownMessage('Shutting down dev ...')
setTimeout(() => {
if (isUnitTest()) return
treeKill(process.pid, 'SIGINT', false, () => {
process.exit(0)
})
}, 2000)
waitForPostRunHookAndExit()
}
clearInterval(pollingInterval.current)
await developerPreview.disable()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ vi.mock('@shopify/cli-kit/node/system', async () => {
})
vi.mock('@shopify/cli-kit/node/context/local')
vi.mock('@shopify/cli-kit/node/tree-kill')
vi.mock('@shopify/cli-kit/node/hooks/postrun', async () => {
const actual: any = await vi.importActual('@shopify/cli-kit/node/hooks/postrun')
return {
...actual,
waitForPostRunHookAndExit: vi.fn(),
}
})

const mocks = vi.hoisted(() => {
return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ import {Box, Text, useInput, useStdin} from '@shopify/cli-kit/node/ink'
import {handleCtrlC} from '@shopify/cli-kit/node/ui'
import {openURL, terminalSupportsHyperlinks} from '@shopify/cli-kit/node/system'
import figures from '@shopify/cli-kit/node/figures'
import {isUnitTest} from '@shopify/cli-kit/node/context/local'
import {treeKill} from '@shopify/cli-kit/node/tree-kill'
import {postRunHookHasCompleted} from '@shopify/cli-kit/node/hooks/postrun'
import {waitForPostRunHookAndExit} from '@shopify/cli-kit/node/hooks/postrun'
import {Writable} from 'stream'

interface DevStatusShortcut extends TabShortcut {
Expand Down Expand Up @@ -70,18 +68,7 @@ const DevSessionUI: FunctionComponent<DevSesionUIProps> = ({
setIsShuttingDownMessage('Shutting down dev ...')
await onAbort()
}
if (isUnitTest()) return

// Wait for the post run hook to complete or timeout after 5 seconds.
let totalTime = 0
setInterval(() => {
if (postRunHookHasCompleted() || totalTime > 5000) {
treeKill(process.pid, 'SIGINT', false, () => {
process.exit(0)
})
}
totalTime += 100
}, 100)
waitForPostRunHookAndExit()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need for the extra comments, seems like we are explaining the same thing in 3 different places.

waitForPostRunHookAndExit already has a description, anyone would understand what it does.

let's remove this comment and the one in Dev.tsx

})

const errorHandledProcesses = useMemo(() => {
Expand Down
39 changes: 38 additions & 1 deletion packages/cli-kit/src/public/node/hooks/postrun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Postrun hook — uses dynamic imports to avoid loading heavy modules (base-command, analytics)
* at module evaluation time. These are only needed after the command has already finished.
*/
import {treeKill} from '../tree-kill.js'
import {Command, Hook} from '@oclif/core'

let postRunHookCompleted = false
Expand All @@ -15,6 +16,42 @@ export function postRunHookHasCompleted(): boolean {
return postRunHookCompleted
}

/**
* Wait for the postrun hook to finish (so auto-upgrade has a chance to run) and then
* tree-kill the current process tree before exiting.
*
* Long-running interactive commands like `app dev` need this when the user terminates
* the command via `q` or Ctrl+C. The dev sub-processes such as servers and watchers keep
* the event loop alive, so even after oclif's postrun hook completes the node process
* won't exit on its own and we have to `treeKill` the process tree. We must not do that
* before the postrun hook has actually finished running auto-upgrade, otherwise we would
* kill the upgrade mid-way while `npm install` is still running.
*
* The flag `postRunHookCompleted` is flipped at the very end of the hook after
* `autoUpgradeIfNeeded` resolves, so polling it here is safe.
*/
export function waitForPostRunHookAndExit(): void {
const pollIntervalMs = 100
// Auto-upgrade can take a while (npm/pnpm/yarn install). Cap the wait generously so
// a stuck upgrade still terminates the process eventually.
const maxWaitMs = 120000

let elapsed = 0
let terminating = false
const handle = setInterval(() => {
if (terminating) return
if (postRunHookHasCompleted() || elapsed >= maxWaitMs) {
terminating = true
clearInterval(handle)
treeKill(process.pid, 'SIGINT', false, () => {
process.exit(0)
})
return
}
elapsed += pollIntervalMs
}, pollIntervalMs)
}

// This hook is called after each successful command run. More info: https://oclif.io/docs/hooks
export const hook: Hook.Postrun = async ({config, Command}) => {
await detectStopCommand(Command as unknown as typeof Command)
Expand All @@ -28,9 +65,9 @@ export const hook: Hook.Postrun = async ({config, Command}) => {
const {outputDebug} = await import('../output.js')
const command = Command.id.replace(/:/g, ' ')
outputDebug(`Completed command ${command}`)
postRunHookCompleted = true

if (!command.includes('notifications') && !command.includes('upgrade')) await autoUpgradeIfNeeded()
postRunHookCompleted = true
}

/**
Expand Down
Loading