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.

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

Block Directory: Store refactor #22388

Merged
merged 31 commits into from
May 25, 2020
Merged

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented May 15, 2020

Description

  • Pulls in some of the work from Block Directory: Add error messages inline. #20001 (can be rebased out once that merges)
  • Cleans up jsdoc descriptions & formatting
  • Switches to using data-controls, which cuts down controls to just the LOAD_ASSETS action
  • Removes uninstall code, so we don't accidentally end up with duplicate code when we re-implement it
  • Refactor the install & download actions to be a single installBlockType action generator, which walks through each step. This avoids needing to add extra logic into the withDispatch for error/success callbacks
  • Updates the notice action/reducer to store the error message itself, which will let us show the user more descriptive error messages.

How 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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@ryelle ryelle added the [Feature] Block Directory Related to the Block Directory, a repository of block plugins label May 15, 2020
@ryelle ryelle self-assigned this May 15, 2020
@github-actions
Copy link

github-actions bot commented May 15, 2020

Size Change: -126 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/api-fetch/index.js 3.4 kB -1 B
build/autop/index.js 2.83 kB +1 B
build/block-directory/index.js 6.48 kB -454 B (7%)
build/block-directory/style-rtl.css 788 B -2 B (0%)
build/block-directory/style.css 788 B -3 B (0%)
build/block-editor/index.js 105 kB -60 B (0%)
build/block-editor/style-rtl.css 10.9 kB +55 B (0%)
build/block-editor/style.css 10.9 kB +53 B (0%)
build/block-library/index.js 119 kB -616 B (0%)
build/block-library/style-rtl.css 7.48 kB -1 B
build/block-library/style.css 7.48 kB -1 B
build/block-serialization-default-parser/index.js 1.88 kB +2 B (0%)
build/components/index.js 190 kB -3 B (0%)
build/components/style-rtl.css 17.1 kB -2 B (0%)
build/components/style.css 17.1 kB -4 B (0%)
build/compose/index.js 9.28 kB +34 B (0%)
build/core-data/index.js 11.4 kB +15 B (0%)
build/data/index.js 8.42 kB +1 B
build/date/index.js 5.47 kB -2 B (0%)
build/edit-navigation/index.js 6.63 kB +3 B (0%)
build/edit-post/index.js 302 kB -5 B (0%)
build/edit-site/index.js 13.8 kB +639 B (4%)
build/edit-site/style-rtl.css 5.53 kB +77 B (1%)
build/edit-site/style.css 5.53 kB +77 B (1%)
build/edit-widgets/index.js 7.87 kB -1 B
build/editor/index.js 44.6 kB -4 B (0%)
build/element/index.js 4.65 kB +1 B
build/format-library/index.js 7.71 kB +71 B (0%)
build/hooks/index.js 2.13 kB +1 B
build/keyboard-shortcuts/index.js 2.51 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -2 B (0%)
build/media-utils/index.js 5.29 kB -1 B
build/nux/index.js 3.4 kB +2 B (0%)
build/rich-text/index.js 14.8 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/blob/index.js 620 B 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-widgets/style-rtl.css 4.58 kB 0 B
build/edit-widgets/style.css 4.57 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

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.

packages/block-directory/src/store/controls.js Outdated Show resolved Hide resolved
@StevenDufresne
Copy link
Contributor

You have this as a draft PR, what else do you plan on refactoring?

@ryelle ryelle force-pushed the try/block-directory-store-refactor branch from a85c0b9 to 2417257 Compare May 19, 2020 19:02
@ryelle ryelle marked this pull request as ready for review May 19, 2020 19:14
@ryelle
Copy link
Contributor Author

ryelle commented May 19, 2020

You have this as a draft PR, what else do you plan on refactoring?

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 DownloadableBlockNotice, since I introduced the getErrorNoticeForBlock selector).

@StevenDufresne
Copy link
Contributor

Just nothing here that the API has some issues that cause 500 errors.
This should be corrected in:
#22454 (or a related PR, @tellyworth has one going).

This is the result:

Error

@TimothyBJacobs
Copy link
Member

@StevenDufresne do you know what API route was called that ended up triggering that error? Want to make sure it gets corrected.

Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

@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.

packages/block-directory/src/store/actions.js Outdated Show resolved Hide resolved
const response = yield apiFetch( {
path: '__experimental/block-directory/install',
data: {
slug: id,
},
method: 'POST',
} );
if ( response.success === false ) {
if ( response.success !== true ) {
throw new Error( response.errorMessage );
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested locally, if the length of this error message is < 1, no error message will be displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should consider a default error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙂

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@StevenDufresne
Copy link
Contributor

Looks good.

@ryelle ryelle force-pushed the try/block-directory-store-refactor branch from 6253471 to 4c406fc Compare May 22, 2020 18:10
Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

I'm good with the changes.

@ryelle ryelle merged commit f717994 into master May 25, 2020
@ryelle ryelle deleted the try/block-directory-store-refactor branch May 25, 2020 23:30
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone May 25, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants