Conversation
|
Size Change: -126 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
packages/block-directory/src/components/downloadable-blocks-list/index.js
Outdated
Show resolved
Hide resolved
StevenDufresne
left a comment
There was a problem hiding this comment.
Awesome. Thanks for the taking the time.
I left a few comments on choices the code already makes. They are not necessary for merge, but worth discussing in the context of a refactor.
|
You have this as a draft PR, what else do you plan on refactoring? |
a85c0b9 to
2417257
Compare
I wanted to rebase it after #20001 was merged. Now that's done, so this is ready for review. I also refactored some of the components & tests (like |
|
Just nothing here that the API has some issues that cause This is the result: |
|
@StevenDufresne do you know what API route was called that ended up triggering that error? Want to make sure it gets corrected. |
StevenDufresne
left a comment
There was a problem hiding this comment.
@StevenDufresne do you know what API route was called that ended up triggering that error? Want to make sure it gets corrected.
This was the result of the plugin already being installed -> /install.
I'm certain it's the result of new missing from WP_Error statements.
| } ); | ||
| if ( response.success === false ) { | ||
| if ( response.success !== true ) { | ||
| throw new Error( response.errorMessage ); |
There was a problem hiding this comment.
I tested locally, if the length of this error message is < 1, no error message will be displayed.
There was a problem hiding this comment.
I wonder if we should consider a default error message?
There was a problem hiding this comment.
Added the oh-so-descriptive "An error occurred." as a default. Hopefully we'll have error content for most detectable errors, so no one will see this 🙂
There was a problem hiding this comment.
If an error occurs here it'd be a proper REST API error response. AFAIK, that throws. I don't think this code path could get executed. Am I missing something?
There was a problem hiding this comment.
Yeah, this would probably never be executed (and if it is, errorMessage wouldn't exist) — but it could if the response from the server was malformed somehow, or the API endpoint could change to return failure info. I've updated it to show a default message, and we can look at removing it if needed when the API settles some.
|
Looks good. |
Includes very basic testing, since I'm having trouble with both promises and document mocking
This walks through each step, and removes the need for onError & onSuccess
This will let us return more descriptive error messages for each kind of error, instead of generic install/download failed messages
Co-authored-by: Steven Dufresne <steve.dufresne@automattic.com>
Behavior is tested in DownloadableBlockListItem tests
This prevents an error where `onSelect` tries to run on a block that fails to install
6253471 to
4c406fc
Compare
StevenDufresne
left a comment
There was a problem hiding this comment.
I'm good with the changes.
Description
data-controls, which cuts downcontrolsto just theLOAD_ASSETSactioninstallBlockTypeaction generator, which walks through each step. This avoids needing to add extra logic into thewithDispatchfor error/success callbacksHow has this been tested?
I've added tests for actions, reducers, selectors, and to some extent the controls helpers. I also did some quick browser-testing— this removes any error notices (because of the switch in 20001), so it's not ready to merge until that's done.
Checklist: