[Themes] theme dev - unify asset upload error reporting
#5209
+9
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage report
Show files with reduced coverage 🔻
Test suite run success2007 tests passing in 906 suites. Report generated by 🧪jest coverage report action from 454ca51 |
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
1447d8a to
5ac4ebb
Compare
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
5ac4ebb to
3c10c59
Compare
jamesmengo
changed the title
theme dev - unify asset upload error reporting
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
9cf51e3 to
7446211
Compare
This comment has been minimized.
This comment has been minimized.
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
7446211 to
f997b11
Compare
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
f997b11 to
2bdf2fb
Compare
This was referenced Jan 15, 2025
graygilmore
reviewed
Jan 16, 2025
| outputWarn(`Failed to upload file ${key}:`) | ||
| outputInfo(`${errorMessage}`) | ||
| outputNewline() | ||
| const errorMessage = result.errors?.asset?.map((err) => `-${err}`).join('\n') ?? 'File upload failed' |
There was a problem hiding this comment.
Since we're in here modifying this code... could we render all of the failed uploads in a single error? Something that looks like:
╭─ failure ────────────────────────────────────────────────────────────────────╮
│ │
│ Failed to upload files: │
│ │
│ • snippets/foo.liquid
| Some reason for the error │
│ │
│ • snippets/foo.liquid
| Some reason for the error │
│ │
│ • snippets/foo.liquid
| Some reason for the error │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯What do you think?
There was a problem hiding this comment.
Yes, though I would prefer to do that outside of this issue since I have am making this quick change to build on top of it.
I am also adding an uploadErrors map on top of this that will enable us to go in the direction you're suggesting!
There was a problem hiding this comment.
I don't think we need uploadErrors for this since we already have the results in uploadResults here but I'm happy enough to push my suggestion off into a follow up.
5 tasks
graygilmore
approved these changes
Jan 16, 2025
| outputWarn(`Failed to upload file ${key}:`) | ||
| outputInfo(`${errorMessage}`) | ||
| outputNewline() | ||
| const errorMessage = result.errors?.asset?.map((err) => `-${err}`).join('\n') ?? 'File upload failed' |
There was a problem hiding this comment.
I don't think we need uploadErrors for this since we already have the results in uploadResults here but I'm happy enough to push my suggestion off into a follow up.
graygilmore
reviewed
Jan 16, 2025
Co-authored-by: Gray Gilmore <graygilmore@gmail.com>
jamesmengo
force-pushed
the
jm/01-15-unify_asset_upload_error_reporting
branch
from
bdd01e4 to
4d5dfae
Compare
frandiox
approved these changes
Jan 16, 2025
Co-authored-by: Fran Dios <fran.dios@shopify.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Improves consistency of how we surface asset upload errors to the user
The
theme devcommand uploads in two placesThe way we report these errors were different, leading to a disjointed visual experience
This PR unifies the way those errors are rendered to the terminal.
This also makes it easier for us to revisit whether we want to render this, or just call
outputWarn. Personally, I feel like this adds noise to the terminal but I wanted to keep this change insulated for the moment. Consistency is more important in the short term.WHAT is this pull request doing?
BEFORE
AFTER
How to test your changes?
theme devcommandPost-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist