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

Prevent network requests related to ephemeral posts in the widgets editor #25899

Merged
merged 4 commits into from Oct 8, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Oct 7, 2020

Description

This PR follows up the discussion from #25831:

Both navigation editor and widgets editor need a top-level entity record to store all the blocks. While post editor may just use the post entity that comes from the API, there is no corresponding concept in the navigation editor or the widgets editors. At the moment both editors use a "fake" post entity record that's never resolved from the API or persisted to the API. This requires carefully dispatching actions to avoid triggering any resolvers. In other words, it's a hack.

The approach taken here is different. Instead of adding a new concept to core-data, this PR simply plugs into api-fetch and prevents any unwanted requests from happening. While the implementation differs, the approach is inspired by Drupal's api-fetch.

The advantage of this approach over the current approach is that it's resilient. It doesn't require any initialization, it doesn't rely on implementation details of the resolution pipeline (such as faking startResolution and endResolution), it won't be easily broken by updates to core-data. The only hardcoded part is the path string, I'd rather not use the one specified in defaultEntities -> baseURL as the data structure there is just another implementation detail of the resolution system.

The same treatment could be applied to the navigation editor which also uses an in-memory post as a convenience wrapper.

How has this been tested?

  1. Go to widgets editor.
  2. Open the network panel and watch out for any 404 and 403.
  3. Confirm it works as before this PR - widgets are loaded, possible to manipulate, saving works fine.
  4. Confirm there weren't any widget-area related network error logged.

Types of changes

Non-breaking change

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.

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels Oct 7, 2020
@adamziel adamziel added this to PRs in progress in Block-based Widgets Editor via automation Oct 7, 2020
@adamziel adamziel self-assigned this Oct 7, 2020
@github-actions
Copy link

github-actions bot commented Oct 7, 2020

Size Change: +10.4 kB (0%)

Total Size: 1.19 MB

Filename Size Change
build/annotations/index.js 3.54 kB +22 B (0%)
build/block-directory/index.js 8.55 kB +1 B
build/block-editor/index.js 129 kB -63 B (0%)
build/block-library/editor-rtl.css 8.65 kB +6 B (0%)
build/block-library/editor.css 8.65 kB +6 B (0%)
build/block-library/index.js 144 kB +9.23 kB (6%) 🔍
build/blocks/index.js 47.5 kB -16 B (0%)
build/components/index.js 169 kB +808 B (0%)
build/components/style-rtl.css 15.4 kB +121 B (0%)
build/components/style.css 15.4 kB +119 B (0%)
build/compose/index.js 9.43 kB +4 B (0%)
build/core-data/index.js 12 kB +11 B (0%)
build/date/index.js 31.9 kB +1 B
build/edit-navigation/index.js 10.6 kB -10 B (0%)
build/edit-post/index.js 306 kB -5 B (0%)
build/edit-site/index.js 20.9 kB +351 B (1%)
build/edit-site/style-rtl.css 3.73 kB +14 B (0%)
build/edit-site/style.css 3.73 kB +12 B (0%)
build/edit-widgets/index.js 21.2 kB -223 B (1%)
build/edit-widgets/style-rtl.css 3.02 kB +19 B (0%)
build/edit-widgets/style.css 3.02 kB +20 B (0%)
build/editor/index.js 45.4 kB -15 B (0%)
build/element/index.js 4.45 kB +5 B (0%)
build/format-library/index.js 7.49 kB +2 B (0%)
build/list-reusable-blocks/index.js 3.02 kB -3 B (0%)
build/nux/index.js 3.27 kB +1 B
build/plugins/index.js 2.44 kB +2 B (0%)
build/redux-routine/index.js 2.85 kB -1 B
build/server-side-render/index.js 2.6 kB +1 B
build/shortcode/index.js 1.7 kB +1 B
build/viewport/index.js 1.74 kB +2 B (0%)
build/warning/index.js 1.14 kB +7 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 668 B 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 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/style-rtl.css 7.66 kB 0 B
build/block-library/style.css 7.65 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.6 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 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.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/rich-text/index.js 13 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I wonder if we can do better than plopping

if ( nextOptions?.responseBody ) {
	return nextOptions.responseBody;
}

like that. Seems random :)

Otherwise the idea to have a way to stop apiFetch via middleware is a good idea.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

It's definitely a nicer hack, but it's still a hack 🙂

I'm happy with it for 5.6. Would prefer that we don't add responseBody though and instead just don't call next().

packages/api-fetch/README.md Outdated Show resolved Hide resolved
@draganescu
Copy link
Contributor

Yes I vote for @noisysocks idea and also call the option something that indicates that apiFetch won't request anything. Setting responseBody is weird :)

@adamziel
Copy link
Contributor Author

adamziel commented Oct 8, 2020

It's definitely a nicer hack, but it's still a hack 🙂

It is still a hack indeed, multi-entity saving/editing would be the real answer here - let's explore it after 5.6 :-)

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I think this is a good fix, albeit not the complete solution, to the problem we have.

@draganescu draganescu merged commit cf03c39 into master Oct 8, 2020
Block-based Widgets Editor automation moved this from PRs in progress to Done Oct 8, 2020
@draganescu draganescu deleted the update/stub-post-middleware branch October 8, 2020 09:44
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants