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

[Themes] Add error overlay for theme upload failures #5208

Closed
wants to merge 4 commits into from
+63 −26
Closed

[Themes] Add error overlay for theme upload failures #5208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac149f9
Add uploadErrors property to theme file system
jamesmengo Jan 15, 2025
26dd68b
Save asset upload failures during theme dev to uploadErrors map
jamesmengo Jan 15, 2025
333b417
Render error overlay when themeFileSystem has upload errors
jamesmengo Jan 15, 2025
762d5aa
PIVOT - Use existing errorPage to render error overlay
jamesmengo Jan 16, 2025
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 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 @@ -99,6 +99,11 @@ export interface ThemeFileSystem extends VirtualFileSystem {
* Applies filters to ignore files from .shopifyignore file, --ignore and --only flags.
*/
applyIgnoreFilters: <T extends {key: string}>(files: T[]) => T[]

/**
* Stores upload errors returned when uploading files via the Asset API
*/
uploadErrors: Map<Key, string[]>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered storing this inside of the ThemeAsset type

I chose to store it here because

  • This type is simple, easy to update, and communicates that this is a temporary store of errors rather than an actual property of the theme asset

- It is likely that we will update this and check if this is empty frequently. We want these operations to be fast and straightforward

  • Checking whether uploadErrors are present is be O(1) since we don't need to read all theme assets
    • We COULD do a two-pronged approach where we store a list of files with errors in a Set, and store the errors themselves on the ThemeAsset. I tried this and it felt clunky.
  • Updates should be fast and easy
    • If we store in ThemeAsset, we'd need to get the asset, check if it exists, and update the error key

}

/**
Expand Down
28 changes: 28 additions & 0 deletions packages/theme/src/cli/utilities/theme-environment/hot-reload/error-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
interface Error {
message: string
code: string
}

export function getErrorPage(options: {title: string; header: string; errors: Error[]}) {
const html = String.raw

return html`<html>
<head>
<title>${options.title ?? 'Unknown error'}</title>
</head>
<body
id="full-error-page"
style="display: flex; flex-direction: column; align-items: center; padding-top: 20px; font-family: Arial"
>
<h1>${options.header}</h1>

${options.errors
.map(
(error) =>
`<p>${error.message}</p>
<pre>${error.code}</pre>`,
)
.join('')}
</body>
</html>`
}
38 changes: 18 additions & 20 deletions packages/theme/src/cli/utilities/theme-environment/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {getProxyStorefrontHeaders, patchRenderingResponse} from './proxy.js'
import {getInMemoryTemplates, injectHotReloadScript} from './hot-reload/server.js'
import {render} from './storefront-renderer.js'
import {getErrorPage} from './hot-reload/error-page.js'
import {getExtensionInMemoryTemplates} from '../theme-ext-environment/theme-ext-server.js'
import {logRequestLine} from '../log-request-line.js'
import {defineEventHandler, getCookie, setResponseHeader, setResponseStatus, type H3Error} from 'h3'
Expand Down Expand Up @@ -31,6 +32,17 @@ export function getHtmlHandler(theme: Theme, ctx: DevServerContext) {

assertThemeId(response, html, String(theme.id))

if (ctx.localThemeFileSystem.uploadErrors.size > 0) {
html = getErrorPage({
title: 'Failed to Upload Theme Files',
header: 'Upload Errors',
errors: Array.from(ctx.localThemeFileSystem.uploadErrors.entries()).map(([file, errors]) => ({
message: file,
code: errors.join('\n'),
})),
})
}

if (ctx.options.liveReload !== 'off') {
html = injectHotReloadScript(html)
}
Expand All @@ -52,8 +64,12 @@ export function getHtmlHandler(theme: Theme, ctx: DevServerContext) {
let errorPageHtml = getErrorPage({
title,
header: title,
message: [...rest, cause?.message ?? error.message].join('<br>'),
code: error.stack?.replace(`${error.message}\n`, '') ?? '',
errors: [
{
message: [...rest, cause?.message ?? error.message].join('<br>'),
code: error.stack?.replace(`${error.message}\n`, '') ?? '',
},
],
})

if (ctx.options.liveReload !== 'off') {
Expand All @@ -65,24 +81,6 @@ export function getHtmlHandler(theme: Theme, ctx: DevServerContext) {
})
}

function getErrorPage(options: {title: string; header: string; message: string; code: string}) {
const html = String.raw

return html`<html>
<head>
<title>${options.title ?? 'Unknown error'}</title>
</head>
<body
id="full-error-page"
style="display: flex; flex-direction: column; align-items: center; padding-top: 20px; font-family: Arial"
>
<h2>${options.header}</h2>
<p>${options.message}</p>
<pre>${options.code}</pre>
</body>
</html>`
}

function assertThemeId(response: Response, html: string, expectedThemeId: string) {
/**
* DOM example:
Expand Down
1 change: 1 addition & 0 deletions packages/theme/src/cli/utilities/theme-fs-empty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function emptyFileSystem<T>(): T {
root: '',
files: new Map(),
unsyncedFileKeys: new Set(),
uploadErrors: new Map(),
ready: () => Promise.resolve(),
delete: async (_: string) => {},
read: async (_: string) => '',
Expand Down
2 changes: 2 additions & 0 deletions packages/theme/src/cli/utilities/theme-fs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ describe('theme-fs', () => {
fsEntry({checksum: '64caf742bd427adcf497bffab63df30c', key: 'templates/404.json'}),
]),
unsyncedFileKeys: new Set(),
uploadErrors: new Map(),
ready: expect.any(Function),
delete: expect.any(Function),
write: expect.any(Function),
Expand All @@ -82,6 +83,7 @@ describe('theme-fs', () => {
root,
files: new Map(),
unsyncedFileKeys: new Set(),
uploadErrors: new Map(),
ready: expect.any(Function),
delete: expect.any(Function),
write: expect.any(Function),
Expand Down
14 changes: 8 additions & 6 deletions 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 @@ -45,6 +45,7 @@ const THEME_PARTITION_REGEX = {

export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOptions): ThemeFileSystem {
const files = new Map<string, ThemeAsset>()
const uploadErrors = new Map<string, string[]>()
const unsyncedFileKeys = new Set<string>()
const filterPatterns = {
ignoreFromFile: [] as string[],
Expand Down Expand Up @@ -147,12 +148,12 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti

const [result] = await bulkUploadThemeAssets(Number(themeId), [{key: fileKey, value: content}], adminSession)

if (!result?.success) {
throw new Error(
result?.errors?.asset
? `\n\n${result.errors.asset.map((error) => `- ${error}`).join('\n')}`
: 'Response was not successful.',
)
if (result?.success) {
uploadErrors.delete(fileKey)
} else {
const errors = result?.errors?.asset ?? ['Response was not successful.']
uploadErrors.set(fileKey, errors)
throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to keep throwing the error here so that we output to the terminal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error(errors.length === 1 ? errors[0] : errors.join('\n'))
throw new Error(errors.join('\n')) 
> ['foo'].join(',')
'foo'

}

unsyncedFileKeys.delete(fileKey)
Expand Down Expand Up @@ -223,6 +224,7 @@ export function mountThemeFileSystem(root: string, options?: ThemeFileSystemOpti
root,
files,
unsyncedFileKeys,
uploadErrors,
ready: () => themeSetupPromise,
delete: async (fileKey: string) => {
files.delete(fileKey)
Expand Down
1 change: 1 addition & 0 deletions packages/theme/src/cli/utilities/theme-fs/theme-fs-mock-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export function fakeThemeFileSystem(
root,
files,
unsyncedFileKeys: new Set(),
uploadErrors: new Map(),
ready: () => Promise.resolve(),
delete: async (fileKey: string) => {
files.delete(fileKey)
Expand Down
Loading