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 editor: refactor effect.js to controls #27298

Merged
merged 4 commits into from
Dec 4, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 26, 2020

Description

This PR migrates the block-editor store effects into controls. See #27069 and #26866.

That allows us to also remove refx as a middleware for side effects, which this PR is also taking care of.

How has this been tested?

  • Unit test modifications - ensuring they pass.
  • The e2e tests continue to pass

Types of changes

Non-breaking code quality enhancement.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Nov 26, 2020

Size Change: +32 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/annotations/index.js 3.8 kB -1 B
build/blob/index.js 665 B +1 B
build/block-directory/index.js 8.72 kB -1 B
build/block-editor/index.js 134 kB +31 B (0%)
build/block-library/index.js 148 kB +1 B
build/components/index.js 172 kB +1 B
build/compose/index.js 9.95 kB +1 B
build/data-controls/index.js 827 B -1 B
build/data/index.js 8.8 kB +2 B (0%)
build/date/index.js 11.2 kB -3 B (0%)
build/deprecated/index.js 769 B +1 B
build/dom/index.js 4.95 kB +1 B
build/edit-navigation/index.js 11.1 kB -2 B (0%)
build/edit-post/index.js 306 kB +3 B (0%)
build/edit-site/index.js 24 kB +1 B
build/editor/index.js 43.3 kB +4 B (0%)
build/element/index.js 4.62 kB -1 B
build/format-library/index.js 6.86 kB -1 B
build/hooks/index.js 2.27 kB -1 B
build/keyboard-shortcuts/index.js 2.54 kB -1 B
build/notices/index.js 1.82 kB +2 B (0%)
build/nux/index.js 3.42 kB +1 B
build/redux-routine/index.js 2.83 kB -4 B (0%)
build/shortcode/index.js 1.69 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.95 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-library/style-rtl.css 8.26 kB 0 B
build/block-library/style.css 8.26 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/dom-ready/index.js 571 B 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/style-rtl.css 3.92 kB 0 B
build/edit-site/style.css 3.92 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@adamziel adamziel changed the title Remove effects.js from block-editor Block editor: remove effect.js Nov 26, 2020
@adamziel adamziel changed the title Block editor: remove effect.js Block editor: refactor effect.js to controls Nov 26, 2020
@adamziel adamziel requested a review from tyxla November 26, 2020 13:03
@adamziel adamziel added [Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement. labels Nov 26, 2020
@adamziel adamziel self-assigned this Nov 26, 2020
@adamziel adamziel force-pushed the try/effects-js-block-editor branch from 3b3917c to ebc5b7d Compare November 27, 2020 09:51
@adamziel
Copy link
Contributor Author

@tyxla would you mind reviewing? E2E failures here are consistent with ones on master.

@gziolo gziolo self-requested a review November 27, 2020 15:27
@gziolo
Copy link
Member

gziolo commented Nov 27, 2020

So rfx is only 577B, but it's nice to see it could go away 😄

It's surprising to see that the size of the bundle doesn't decrease though:

build/block-editor/index.js | 134 kB | +31 B (0%)

@gziolo gziolo requested a review from nerrad November 28, 2020 06:54
@tyxla
Copy link
Member

tyxla commented Nov 30, 2020

@adamziel I noticed you've started working on #27276 - are we still planning to migrate to action generators, or are we abandoning the idea? Just wanted to make sure we haven't changed course, and if this PR is still something we're considering.

@adamziel
Copy link
Contributor Author

adamziel commented Dec 1, 2020

@tyxla #27276 is exploratory, let's not block on it. We want to get rid of effects.js with or without it. Migrating this change to async/await should be as easy as search/replace

@gziolo
Copy link
Member

gziolo commented Dec 1, 2020

I closed #14594 from @nerrad which was quite old in favor of this PR. Let's land this one sooner than later 😅

Copy link
Member

@gziolo gziolo 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 not the best person to approve this PR as I know little about side effects in the block editor, but based on my limited knowledge changes proposed look good. All tests pass which is the most important indicator here.

I would appreciate some cross-check from someone else.

'getSelectedBlockCount'
);

speak(
Copy link
Member

Choose a reason for hiding this comment

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

It isn't clear when we use controls in actions. Technically speaking speak is also a side effect, although it definitely isn't something that you need to wait for. I see that it's inlined the same way in other places so it might be may misunderstanding when to use controls.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is testing well for me and code looks good. Really nice work 👏

I can see some experimental e2e tests are failing - any chance that's related?

@gziolo
Copy link
Member

gziolo commented Dec 4, 2020

Those tests shouldn't fail anymore after the rebase. I can't guarantee something else will fail though. The stability of e2e tests isn't ideal with the constantly growing number of contributions :)

@adamziel adamziel merged commit 87171eb into master Dec 4, 2020
@adamziel adamziel deleted the try/effects-js-block-editor branch December 4, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants