-
Notifications
You must be signed in to change notification settings - Fork 219
Fix theme dev failing analytics #6605
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.
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
Changes from all commits
2b7d391
74ca3e2
0730a88
c72ce03
9c35fb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| interface PromiseWithResolvers<T> { | ||
| promise: Promise<T> | ||
| resolve: (value: T | PromiseLike<T>) => void | ||
| reject: (reason?: unknown) => void | ||
| } | ||
|
|
||
| // Polyfill for Promise.withResolvers | ||
| // Can remove once our minimum supported Node version is 22 | ||
| export 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} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,23 +119,7 @@ export async function dev(options: DevOptions) { | |
| }, | ||
| } | ||
|
|
||
| if (options['theme-editor-sync']) { | ||
| session.storefrontPassword = await storefrontPasswordPromise | ||
| } | ||
|
|
||
| const {serverStart, renderDevSetupProgress} = setupDevServer(options.theme, ctx) | ||
|
|
||
| if (!options['theme-editor-sync']) { | ||
| session.storefrontPassword = await storefrontPasswordPromise | ||
| } | ||
|
|
||
| await renderDevSetupProgress() | ||
| await serverStart() | ||
|
|
||
| renderLinks(urls) | ||
| if (options.open) { | ||
| openURLSafely(urls.local, 'development server') | ||
| } | ||
| const {serverStart, renderDevSetupProgress, backgroundJobPromise} = setupDevServer(options.theme, ctx) | ||
|
|
||
| readline.emitKeypressEvents(process.stdin) | ||
| if (process.stdin.isTTY) { | ||
|
|
@@ -167,6 +151,18 @@ export async function dev(options: DevOptions) { | |
| break | ||
| } | ||
| }) | ||
|
|
||
| await Promise.all([ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is there a downside to this? Could a user end up with thousands of open promises that never resolve?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question! In our case, no we wouldn't. The initial promise created with the |
||
| backgroundJobPromise, | ||
| renderDevSetupProgress() | ||
| .then(serverStart) | ||
| .then(() => { | ||
| renderLinks(urls) | ||
| if (options.open) { | ||
| openURLSafely(urls.local, 'development server') | ||
| } | ||
| }), | ||
| ]) | ||
| } | ||
|
|
||
| export function openURLSafely(url: string, label: string) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,16 +5,19 @@ import {getProxyHandler} from './proxy.js' | |
| import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js' | ||
| import {uploadTheme} from '../theme-uploader.js' | ||
| import {renderTasksToStdErr} from '../theme-ui.js' | ||
| import {createAbortCatchError} from '../errors.js' | ||
| import {renderThrownError} from '../errors.js' | ||
| import {promiseWithResolvers} from '../../polyfills/promiseWithResolvers.js' | ||
| import {createApp, defineEventHandler, defineLazyEventHandler, toNodeListener, handleCors} from 'h3' | ||
| import {fetchChecksums} from '@shopify/cli-kit/node/themes/api' | ||
| import {createServer} from 'node:http' | ||
| import type {Checksum, Theme} from '@shopify/cli-kit/node/themes/types' | ||
| import type {DevServerContext} from './types.js' | ||
|
|
||
| export function setupDevServer(theme: Theme, ctx: DevServerContext) { | ||
| const {promise: backgroundJobPromise, reject: rejectBackgroundJob} = promiseWithResolvers<never>() | ||
|
|
||
| const watcherPromise = setupInMemoryTemplateWatcher(theme, ctx) | ||
| const envSetup = ensureThemeEnvironmentSetup(theme, ctx) | ||
| const envSetup = ensureThemeEnvironmentSetup(theme, ctx, rejectBackgroundJob) | ||
| const workPromise = Promise.all([watcherPromise, envSetup.workPromise]).then(() => | ||
| ctx.localThemeFileSystem.startWatcher(theme.id.toString(), ctx.session), | ||
| ) | ||
|
|
@@ -25,31 +28,43 @@ export function setupDevServer(theme: Theme, ctx: DevServerContext) { | |
| serverStart: server.start, | ||
| dispatchEvent: server.dispatch, | ||
| renderDevSetupProgress: envSetup.renderProgress, | ||
| backgroundJobPromise, | ||
| } | ||
| } | ||
|
|
||
| function ensureThemeEnvironmentSetup(theme: Theme, ctx: DevServerContext) { | ||
| const abort = createAbortCatchError('Failed to perform the initial theme synchronization.') | ||
| function ensureThemeEnvironmentSetup( | ||
| theme: Theme, | ||
| ctx: DevServerContext, | ||
| rejectBackgroundJob: (reason?: unknown) => void, | ||
| ) { | ||
| const abort = (error: Error): never => { | ||
| renderThrownError('Failed to perform the initial theme synchronization.', error) | ||
| rejectBackgroundJob(error) | ||
| // Return a never-resolving promise to stop this promise chain without throwing. | ||
| // Throwing would trigger catch handlers and continue execution. This stops the | ||
| // chain while the error is handled through the separate backgroundJobPromise channel. | ||
| return new Promise<never>(() => {}) as never | ||
| } | ||
|
|
||
| const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session).catch(abort) | ||
| const remoteChecksumsPromise = fetchChecksums(theme.id, ctx.session) | ||
|
Comment on lines
-34
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thiiiink we still need the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I double checked and when |
||
|
|
||
| const reconcilePromise = remoteChecksumsPromise | ||
| .then((remoteChecksums) => handleThemeEditorSync(theme, ctx, remoteChecksums)) | ||
| .catch(abort) | ||
| const reconcilePromise = remoteChecksumsPromise.then((remoteChecksums) => | ||
| handleThemeEditorSync(theme, ctx, remoteChecksums, rejectBackgroundJob), | ||
| ) | ||
|
|
||
| const uploadPromise = reconcilePromise | ||
| .then(async ({updatedRemoteChecksumsPromise}) => { | ||
| const updatedRemoteChecksums = await updatedRemoteChecksumsPromise | ||
| return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { | ||
| nodelete: ctx.options.noDelete, | ||
| deferPartialWork: true, | ||
| backgroundWorkCatch: abort, | ||
| }) | ||
| const uploadPromise = reconcilePromise.then(async ({updatedRemoteChecksumsPromise}) => { | ||
| const updatedRemoteChecksums = await updatedRemoteChecksumsPromise | ||
| return uploadTheme(theme, ctx.session, updatedRemoteChecksums, ctx.localThemeFileSystem, { | ||
| nodelete: ctx.options.noDelete, | ||
| deferPartialWork: true, | ||
| backgroundWorkCatch: abort, | ||
| }) | ||
| .catch(abort) | ||
| }) | ||
|
|
||
| const workPromise = uploadPromise.then((result) => result.workPromise).catch(abort) | ||
|
|
||
| return { | ||
| workPromise: uploadPromise.then((result) => result.workPromise).catch(abort), | ||
| workPromise, | ||
| renderProgress: async () => { | ||
| if (ctx.options.themeEditorSync) { | ||
| const {workPromise} = await reconcilePromise | ||
|
|
@@ -74,16 +89,24 @@ function handleThemeEditorSync( | |
| theme: Theme, | ||
| ctx: DevServerContext, | ||
| remoteChecksums: Checksum[], | ||
| rejectBackgroundJob: (reason?: unknown) => void, | ||
| ): Promise<{ | ||
| updatedRemoteChecksumsPromise: Promise<Checksum[]> | ||
| workPromise: Promise<void> | ||
| }> { | ||
| if (ctx.options.themeEditorSync) { | ||
| return reconcileAndPollThemeEditorChanges(theme, ctx.session, remoteChecksums, ctx.localThemeFileSystem, { | ||
| noDelete: ctx.options.noDelete, | ||
| ignore: ctx.options.ignore, | ||
| only: ctx.options.only, | ||
| }) | ||
| return reconcileAndPollThemeEditorChanges( | ||
| theme, | ||
| ctx.session, | ||
| remoteChecksums, | ||
| ctx.localThemeFileSystem, | ||
| { | ||
| noDelete: ctx.options.noDelete, | ||
| ignore: ctx.options.ignore, | ||
| only: ctx.options.only, | ||
| }, | ||
| rejectBackgroundJob, | ||
| ) | ||
| } else { | ||
| return Promise.resolve({ | ||
| updatedRemoteChecksumsPromise: Promise.resolve(remoteChecksums), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the
if optionsandif !optionsfortheme-editor-syncas we are now awaitingstorefrontPasswordPromiseearlier in the code and this is no longer necessary.