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

Fix crash when registering non-core blocks #5982

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jul 20, 2023

Issue

With WordPress/gutenberg#52417, now blocks are memoized in useEntityBlockEditor hook. This hook is used in the EditorProvider component, so the first call is made right after the editor is initialized. When the initial content is parsed here, it processes the blocks based on the block types registered at that time.

The Jetpack blocks are registered on the root component's componentDidMount handler, which is executed after the useEntityBlockEditor hook is invoked the first time. Therefore, when parsing the initial content Jetpack blocks are marked as missing blocks, as they haven't been registered yet by that time.

// Hook triggered after the editor is rendered
addAction( 'native.render', 'gutenberg-mobile-jetpack', ( props ) => {
registerJetpackBlocks( props );
} );

Example content parsed:

[
  {
    "attributes": { "content": "VideoPress block:", "dropCap": false },
    "clientId": "48445782-74e3-44a7-8d42-a4f75cc783f5",
    "innerBlocks": [],
    "isValid": true,
    "name": "core/paragraph",
    "originalContent": "<p>VideoPress block:</p>",
    "validationIssues": []
  },
  {
    "attributes": {
      "originalContent": "<!-- wp:videopress/video {\"title\":\"file_example_mp4_480_1_5mg\",\"description\":\"\",\"id\":6037,\"guid\":\"6gn6ukfN\",\"privacySetting\":2,\"allowDownload\":false,\"rating\":\"G\",\"isPrivate\":true,\"duration\":30466} /-->",
      "originalName": "videopress/video",
      "originalUndelimitedContent": ""
    },
    "clientId": "5c9a01f4-35e2-42ad-94e4-bc20025523dc",
    "innerBlocks": [],
    "isValid": true,
    "name": "core/missing",
    "originalContent": "<!-- wp:videopress/video {\"title\":\"file_example_mp4_480_1_5mg\",\"description\":\"\",\"id\":6037,\"guid\":\"6gn6ukfN\",\"privacySetting\":2,\"allowDownload\":false,\"rating\":\"G\",\"isPrivate\":true,\"duration\":30466} /-->",
    "validationIssues": []
  }
]

When the Missing blocks are rendered, this line, which calculates the label for the block, produces the crash. Seems that by that time, the proper Jetpack block is fetched for the originalBlockType variable. However, those don't have the settings attribute defined.

WordPress/gutenberg#52417 actually uncovered an issue regarding how we register Jetpack blocks, as we were registering them probably too late in the initialization cycle.

Solution

The workaround for this is to add a new WP hook that allows Gutenberg Mobile to register non-core blocks, like Jetpack blocks, right after the core blocks are registered. This way we ensure that all blocks are registered sequentially (first core blocks, then non-core blocks) before any other calculations are made within the editor.

To test

  1. Create a post.
  2. Add Jetpack blocks (e.g. VideoPress block).
  3. Save the post and close the editor.
  4. Re-open the post.
  5. Observe that no crash is produced.
  6. Observe that none of the blocks are rendered as missing.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Nice approach to fix this regression 🚀 The Gutenberg side of changes were approved in WordPress/gutenberg#52791 (review)

I tested different blocks and it's working as expected on both iOS and Android using the host apps.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 20, 2023

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@fluiddot fluiddot enabled auto-merge July 20, 2023 16:44
@fluiddot fluiddot disabled auto-merge July 20, 2023 17:13
@fluiddot
Copy link
Contributor Author

fluiddot commented Jul 20, 2023

For some reason, all jobs are shown here as "waiting", however, they are running in CircleCI and Buildkite 🤔.

UPDATE: I ended up pushing an empty commit just to trigger CI jobs.

@fluiddot fluiddot enabled auto-merge July 20, 2023 17:25
@fluiddot fluiddot merged commit 1073588 into trunk Jul 20, 2023
@fluiddot fluiddot deleted the fix/crash-when-registering-non-core-blocks branch July 20, 2023 17:57
@derekblank derekblank mentioned this pull request Jul 21, 2023
4 tasks
@fluiddot fluiddot added this to the 1.100.0 (22.9) milestone Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants