Fix theme dev failing analytics

[EvilGenius13]

WHY are these changes introduced?

Closes: https://github.com/Shopify/developer-tools-team/issues/902

Analytics aren't logging when we have a theme dev error during bootup or a background promise rejection during a session. This is because we call process.exit(1) (process.kill), which ends the program immediately before the finally block in theme-command.ts can fire off the analytics event.

WHAT is this pull request doing?

Implementing a deferred promise pattern using Promise.withResolvers(). This is new to Node 22, but I've polyfilled it since we still support Node 18+ as a minimum version.

Promise.withResolvers allows us to pass around resolve/reject around and have it called outside of the original promise. This lets us create a background promise that will only be called when we have an error.

How it works:
Promise.withResolvers() returns an object with three properties:

• promise - a standard promise
• resolve - a function to resolve that promise
• reject - a function to reject that promise

These resolve/reject functions can be passed around and called from anywhere in the code, not just inside the promise. This lets us create a "control promise" that waits indefinitely unless we explicitly reject it.

The flow:

1. User runs theme dev
2. dev.ts calls setupDevServer() in theme-environment.ts
3. In setupDevServer(): We create a deferred promise that never resolves, only rejects on fatal errors:
   • const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers<never>()
4. We pass the rejectBackgroundJob function down through multiple layers:
   • setupDevServer() → ensureThemeEnvironmentSetup() → handleThemeEditorSync() → reconcileAndPollThemeEditorChanges() → pollThemeEditorChanges()
5. Each layer uses this reject function instead of calling process.exit(1) when fatal errors occur
6. Back in dev.ts: We await both the server setup AND the background promise:

   await Promise.all([
          backgroundJobPromise // Hangs forever unless reject() is called    
          renderDevSetupProgress().then(serverStart).then(...)   
  ])

7. Since the background promise never resolves, Promise.all() waits forever keeping the function alive. If an error occurs, rejectBackgroundJob(error) is called, which:

• Rejects the backgroundJobPromise
• Causes Promise.all() to reject
• Bubbles the error up to theme-command.ts
• Triggers the finally block → analytics get logged
• Process exits gracefully

How to test your changes?

In theme-command.ts add some logging to the finally block for analytics

  finally {
     console.log('🔍 Analytics logging...')
     await this.logAnalyticsData(session)
     console.log('✅ Analytics logged!')
   }

In theme-environment.ts replace the const remoteCheckSumsPromise with

const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session)
     .then(() => { throw new Error('TEST: Force failure') })

Run theme dev

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

• n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
• Existing analytics will cater for this addition
• PR includes analytics changes to measure impact

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes

[EvilGenius13]

I removed the if options and if !options for theme-editor-sync as we are now awaiting storefrontPasswordPromise earlier in the code and this is no longer necessary.

[EvilGenius13]

This stops all the layers using the reject from individually outputting the error and polluting the terminal.

[frandiox]

Do you know which part of the code starts showing errors if we don't return a promise here? Is it the ctx.localThemeFileSystem.startWatcher that we can within setupDevServer?

[EvilGenius13]

This is mainly because we have all the catch blocks and in our abort method we throw, so we throw, the next .catch grabs it and calls the abort error and so on.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	79.24% (+0.01% 🔼)	13589/17150
🟡	Branches	73.13% (+0.02% 🔼)	6630/9066
🟡	Functions	79.35% (-0.02% 🔻)	3508/4421
🟡	Lines	79.59% (+0.01% 🔼)	12833/16124

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / loader.ts	93.86%	86.7% (-0.13% 🔻)	97.06%	94.67%
🟢	... / ConcurrentOutput.tsx	98.36% (-1.64% 🔻)	92% (-4% 🔻)	100%	98.33% (-1.67% 🔻)
🔴	... / dev.ts	15% (+1.67% 🔼)	3.33% (+0.39% 🔼)	50% (-7.14% 🔻)	15% (+1.67% 🔼)
🟡	... / theme-environment.ts	69.81% (-1.62% 🔻)	50%	61.9% (+3.08% 🔼)	69.23% (-2.2% 🔻)
🟡	... / theme-polling.ts	67.12% (-0.93% 🔻)	68.75%	78.57%	66.67% (-0.98% 🔻)

Test suite run success

3356 tests passing in 1372 suites.

Report generated by 🧪jest coverage report action from 781c678

[EvilGenius13]

/snapit

[github-actions]

🫰✨ Thanks @EvilGenius13! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

[frandiox]

Good job! This was precisely the suggestion 💯

One more thing for you to think about it: if we don't like passing the reject callback down multiple levels, an alternative for this case would be relying on AsyncLocalStorage.

Basically, you'd pass the reject callback in the context as something like

const {promise, reject} = Promise.withResolvers();
const {serverStart, renderDevSetupProcess} = await asyncStorage.run({exitProcess: reject}, () => setupDevServer(...))

await Promise.all([promise, serverStart().then(...)])

And anywhere in the logic we could run asyncLocalStorage.getStore().exitProcess(error).

In fact, maybe this is something that should go levels above in the same place we define our try/catch wrapper for the analytics: that logic would provide a context that gives access to an infinite promise in case the process should never end (like here when we run a server), and a reject callback to end the process in a controlled way.

Not saying we should do this, but perhaps worth considering it in this situation 👍

[frandiox]

I thiiiink we still need the .catch(abort) in this one in case it fails fetching checksums?
You can make a test and mock fetcChecksums return to be a promise rejected after a timeout and see if it works or not 👍

[EvilGenius13]

I double checked and when fetchChecksums fails it passes down to the reconcilePromise and rejects properly!

[frandiox]

Do you know which part of the code starts showing errors if we don't return a promise here? Is it the ctx.localThemeFileSystem.startWatcher that we can within setupDevServer?

[EvilGenius13]

Good job! This was precisely the suggestion 💯

One more thing for you to think about it: if we don't like passing the reject callback down multiple levels, an alternative for this case would be relying on AsyncLocalStorage.

Basically, you'd pass the reject callback in the context as something like

const {promise, reject} = Promise.withResolvers();
const {serverStart, renderDevSetupProcess} = await asyncStorage.run({exitProcess: reject}, () => setupDevServer(...))

await Promise.all([promise, serverStart().then(...)])

And anywhere in the logic we could run asyncLocalStorage.getStore().exitProcess(error).

In fact, maybe this is something that should go levels above in the same place we define our try/catch wrapper for the analytics: that logic would provide a context that gives access to an infinite promise in case the process should never end (like here when we run a server), and a reject callback to end the process in a controlled way.

Not saying we should do this, but perhaps worth considering it in this situation 👍

Okay I checked it out. This is a super neat idea as well. IMO I feel like the current promise passdown method will be a bit easier to debug if we ever need compared to exiting via local storage where it won't be as easily traceable.

If it's okay with you I'd like to keep the current implementation.

[github-actions]

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

[dejmedus]

🎩 went great! I've had the snapbuild dev server running since yesterday, it stayed up and seems to be working exactly as I'd expect. I also built the branch and tried to force some errors and was able to see logAnalyticsData now being called

[EvilGenius13]

Do you know which part of the code starts showing errors if we don't return a promise here? Is it the ctx.localThemeFileSystem.startWatcher that we can within setupDevServer?

Without the extra promise, the rejection propagates through each of the rejectPromise areas which causes the error to render multiple times. This promise stop the initial chain and creates a single spot where we throw the error and you see it rendered in the terminal. I added a better comment above it to explain why it's there!