Fix theme dev failing analytics #6605
Conversation
| if (options.open) { | ||
| openURLSafely(urls.local, 'development server') | ||
| } | ||
| const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx) |
There was a problem hiding this comment.
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.
| const abort = (error: Error): never => { | ||
| renderThrownError('Failed to perform the initial theme synchronization.', error) | ||
| rejectBackgroundJob(error) | ||
| // Return a permanently pending promise to stop execution without throwing |
There was a problem hiding this comment.
This stops all the layers using the reject from individually outputting the error and polluting the terminal.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3423 tests passing in 1390 suites. Report generated by 🧪jest coverage report action from 9c35fb1 |
|
/snapit |
|
🫰✨ 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 |
frandiox
left a comment
frandiox
left a comment
There was a problem hiding this comment.
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 👍
| const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session).catch(abort) | ||
| const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session) |
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
I double checked and when fetchChecksums fails it passes down to the reconcilePromise and rejects properly!
| const abort = (error: Error): never => { | ||
| renderThrownError('Failed to perform the initial theme synchronization.', error) | ||
| rejectBackgroundJob(error) | ||
| // Return a permanently pending promise to stop execution without throwing |
There was a problem hiding this comment.
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?
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. |
|
We detected some changes at 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
left a comment
dejmedus
left a comment
There was a problem hiding this comment.
🎩 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
force-pushed
the
fix-theme-dev-failing-analytics
branch
from
2a15aa5 to
781c678
Compare
Without the extra promise, the rejection propagates through each of the |
| // Polyfill for Promise.withResolvers | ||
| // Can remove once our minimum supported Node version is 22 | ||
| interface PromiseWithResolvers<T> { | ||
| promise: Promise<T> | ||
| resolve: (value: T | PromiseLike<T>) => void | ||
| reject: (reason?: unknown) => void | ||
| } | ||
|
|
||
| function promiseWithResolvers<T>(): PromiseWithResolvers<T> { | ||
| if (typeof Promise.withResolvers === 'function') { | ||
| return Promise.withResolvers<T>() | ||
| } | ||
|
|
||
| let resolve!: (value: T | PromiseLike<T>) => void | ||
| let reject!: (reason?: unknown) => void | ||
| const promise = new Promise<T>((_resolve, _reject) => { | ||
| resolve = _resolve | ||
| reject = _reject | ||
| }) | ||
|
|
||
| return {promise, resolve, reject} | ||
| } | ||
|
|
There was a problem hiding this comment.
(nit) Might be worth chucking this whole function into a separate file. Something like packages/theme/src/cli/polyfills/promise-with-resolvers.ts. Makes it even more explicitly that this can be temporary.
There was a problem hiding this comment.
Good idea, I moved it!
| } | ||
| }) | ||
|
|
||
| await Promise.all([ |
There was a problem hiding this comment.
Since the background promise never resolves, Promise.all() waits forever keeping the function alive
Is there a downside to this? Could a user end up with thousands of open promises that never resolve?
There was a problem hiding this comment.
Great question! In our case, no we wouldn't. The initial promise created with the PromiseWithResolver is passed around and used to reject errors. Every time a file changes or something like a hot-reload, it's all using the same reject function so they don't create a new error. This promise will just hold as a no-op (assuming no error) until you force quit.
EvilGenius13
force-pushed
the
fix-theme-dev-failing-analytics
branch
from
781c678 to
e15628f
Compare
EvilGenius13
force-pushed
the
fix-theme-dev-failing-analytics
branch
from
8da5fd1 to
9c35fb1
Compare
4 participants
WHY are these changes introduced?
Closes: https://github.com/Shopify/developer-tools-team/issues/902
Analytics aren't logging when we have a
theme deverror during bootup or a background promise rejection during a session. This is because we callprocess.exit(1)(process.kill), which ends the program immediately before thefinallyblock intheme-command.tscan 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.withResolversallows us to pass aroundresolve/rejectaround 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: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:
theme devdev.tscallssetupDevServer()intheme-environment.tssetupDevServer(): We create a deferred promise that never resolves, only rejects on fatal errors:const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers<never>()rejectBackgroundJobfunction down through multiple layers:setupDevServer() → ensureThemeEnvironmentSetup() → handleThemeEditorSync() → reconcileAndPollThemeEditorChanges() → pollThemeEditorChanges()process.exit(1)when fatal errors occurPromise.all()waits forever keeping the function alive. If an error occurs,rejectBackgroundJob(error)is called, which:How to test your changes?
In
theme-command.tsadd some logging to thefinallyblock for analyticsIn
theme-environment.tsreplace theconst remoteCheckSumsPromisewithRun
theme devPost-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist