[Themes] Account for empty assets when calculating theme asset size #4475
Conversation
This comment has been minimized.
This comment has been minimized.
jamesmengo
added
the
#gsd:40767
Coverage report
Show files with reduced coverage 🔻
Test suite run success1889 tests passing in 857 suites. Report generated by 🧪jest coverage report action from 3756ce8 |
| @@ -52,7 +52,9 @@ export function buildThemeAsset(asset?: RemoteAssetResponse): ThemeAsset | undef | |||
|
|
|||
| const {key, checksum, attachment, value} = asset | |||
| // Note: for attachments, this is the size of the base64 string, not the real length of the file | |||
| const stats = {size: (value || attachment).length, mtime: Date.now()} | |||
There was a problem hiding this comment.
Oh, I thought they'd be empty string at this point. You can also just do:
const stats = {size: (value || attachment || '').length, mtime: Date.now()}There was a problem hiding this comment.
In fact, the types suggest these are already strings... I guess we are passing undefined as any directly from what we get from the cloud, thus bypassing TS checks?
In that case, perhaps we should implement the fix in the caller of this function to ensure it sends strings.
In any case, making it more robust here doesn't hurt 👍
There was a problem hiding this comment.
Indeed, possibly making value and attachment nullable in the RemoteAssetResponse would represent a bit better their meaning.
| @@ -52,7 +52,9 @@ export function buildThemeAsset(asset?: RemoteAssetResponse): ThemeAsset | undef | |||
|
|
|||
| const {key, checksum, attachment, value} = asset | |||
| // Note: for attachments, this is the size of the base64 string, not the real length of the file | |||
| const stats = {size: (value || attachment).length, mtime: Date.now()} | |||
There was a problem hiding this comment.
Indeed, possibly making value and attachment nullable in the RemoteAssetResponse would represent a bit better their meaning.
jamesmengo
force-pushed
the
jm/fixpull
branch
from
b29a932 to
3756ce8
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/themes/factories.d.ts@@ -10,7 +10,7 @@ interface RemoteThemeResponse {
interface RemoteAssetResponse {
key: string;
checksum: string;
- attachment: string;
+ attachment: string | undefined;
value: string;
}
export interface RemoteBulkUploadResponse {
|
WHY are these changes introduced?
Fixes https://github.com/Shopify/develop-advanced-edits/issues/339
Fixes #4470
WHAT is this pull request doing?
How to test your changes?
shopify theme pull -t <THEME>Measuring impact
How do we know this change was effective? Please choose one:
Checklist