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/170: wait for DOM manipulations before saving post #171

Closed
wants to merge 9 commits into from

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Jun 20, 2024

Description of the Change

The issue happens because when images are added via replaceBlocks(), it takes some time for the browser to first inject <figure /> and then append <img /> to the figure, and since this is an asynchronous process, the client.save() triggers before all the DOM manipulations inside the blocks are completed.

This PR implements MutationObserver to detect if there are any DOM modifications within the replaced block and waits until all DOM are done before saving the post.

Closes #170

How to test the Change

  1. Activate classic editor.
  2. Create 2 posts with gallery and add heading. (Just to test if the PR works as expected to transform other elements to blocks).
  3. Disable the classic editor.
  4. Run the following: wp convert-to-blocks start --post_type=post
  5. Open the URL displayed in the terminal.
  6. It will start converting blocks in both the posts.
  7. Wait until terminal shows the message: "Migration finished successfully."
  8. Open both the posts and reload the page to verify if the transformed blocks are visible as expected.

Changelog Entry

Fixed - Issue with saving post before the convert to blocks transform completed.

Credits

Props @mdesplenter @Sidsector9 @dsawardekar

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@Sidsector9 Sidsector9 removed the request for review from jeffpaul June 20, 2024 15:07
@jeffpaul jeffpaul added this to the 1.4.0 milestone Jun 20, 2024
@Sidsector9 Sidsector9 marked this pull request as draft June 20, 2024 15:55
dkotter
dkotter previously approved these changes Jun 20, 2024
Copy link
Collaborator

@dsawardekar dsawardekar left a comment

Choose a reason for hiding this comment

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

There is likely something

this.didTransform = true;
}
} catch (e) {
// Do nothing
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like it will silently hide errors during block transformation. Do we need the try-catch here?

@dsawardekar
Copy link
Collaborator

I tested this on a hybrid site migration that mixes blocks & CE, and found that it stalls silently instead of moving to the next post. I'll try to to take a look next week.

@Sidsector9
Copy link
Member Author

@dsawardekar I found the root cause of this issue. When images are added via replaceBlocks(), it takes some time for the browser to first inject <figure /> and then append <img /> to the figure, and since this is an asynchronous process, the client.save() triggers before all the HTML injecting is done.

I was able to fix this using Mutation Observer, saving after detecting changes in subtree but I've done this outside in my own plugin for trials. I'll try next week to implement the same here.

@Sidsector9 Sidsector9 changed the title fix/170: check dispatch promise status before saving fix/170: wait for DOM manipulations before saving post Jun 22, 2024
@Sidsector9 Sidsector9 force-pushed the fix/170 branch 2 times, most recently from 4eb6ca8 to d6dc014 Compare June 22, 2024 19:00
@Sidsector9 Sidsector9 marked this pull request as ready for review June 22, 2024 19:12
@jeffpaul
Copy link
Member

@mdesplenter if you're able to test and see if this helps with your issue, that'd be helpful, thanks!

@mdesplenter
Copy link

mdesplenter commented Jun 26, 2024

@jeffpaul Just tested it. When there is nothing to transform it goes to the next post as expected, but when it does have to transform blocks it won't save and doesn't continue. Using breakpoint in the console it stopped on "const result = await transformer.execute();"

@Sidsector9
Copy link
Member Author

@mdesplenter thanks for testing it. I have tested this with 2 posts, both starting with the <h2 /> tags followed by a gallery. Can you share what kind of post content you have? It'll help us debug it further.

It's working for me, please watch the video below:

Screen-2024-06-27-123901.AM.mp4

@mdesplenter
Copy link

mdesplenter commented Jun 27, 2024

The script never reaches line 79 of ClassicBlockTransformer: this.didTransform = true;
It awaits a lot of promises and returns to await Promise.all(promises); from the convertBlocks function.

This is some example code (will remove it later)

Inleiding

title

Cras id nibh aliquet, ullamcorper dolor at, rhoncus erat. Praesent in iaculis enim. Duis consectetur ultricies mi, malesuada convallis est. Mauris ultrices, odio vitae dignissim vulputate, massa odio feugiat mauris, sed pretium mauris nunc sit amet ex. Proin faucibus elit pulvinar eros euismod, at malesuada sem maximus. Curabitur non cursus risus, vitae pharetra ipsum. Sed eu pellentesque nibh. Nullam urna sem, feugiat quis felis nec, sodales tincidunt est. Duis fermentum arcu nec blandit eleifend. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia curae;

[gallery columns="1" size="full" ids="35714"]

[gallery columns="4" size="full" ids="35717,35718,35719,35720"]

title

Cras id nibh aliquet, ullamcorper dolor at, rhoncus erat. Praesent in iaculis enim. Duis consectetur ultricies mi, malesuada convallis est. Mauris ultrices, odio vitae dignissim vulputate, massa odio feugiat mauris, sed pretium mauris nunc sit amet ex. Proin faucibus elit pulvinar eros euismod, at malesuada sem maximus. Curabitur non cursus risus, vitae pharetra ipsum. Sed eu pellentesque nibh. Nullam urna sem, feugiat quis felis nec, sodales tincidunt est. Duis fermentum arcu nec blandit eleifend. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia curae;

Vivamus ac luctus dui. Nunc lacinia sollicitudin ante, sit amet tristique tortor consequat nec. Donec ut cursus libero. Etiam at massa condimentum, condimentum lacus nec, varius arcu. Suspendisse vitae nulla non odio ullamcorper molestie. Aliquam erat dolor, fermentum vel condimentum nec, porttitor ut mauris. Praesent vitae diam at mi accumsan ultrices a ut purus. Pellentesque tristique accumsan elit, non rutrum metus egestas sed.

[gallery columns="1" size="full" ids="35721"]

[gallery columns="2" size="full" ids="35735,35722"]

Title , another one

Cras id nibh aliquet, ullamcorper dolor at, rhoncus erat. Praesent in iaculis enim. Duis consectetur ultricies mi, malesuada convallis est. Mauris ultrices, odio vitae dignissim vulputate, massa odio feugiat mauris, sed pretium mauris nunc sit amet ex. Proin faucibus elit pulvinar eros euismod, at malesuada sem maximus. Curabitur non cursus risus, vitae pharetra ipsum. Sed eu pellentesque nibh. Nullam urna sem, feugiat quis felis nec, sodales tincidunt est. Duis fermentum arcu nec blandit eleifend. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia curae;

Vivamus ac luctus dui. Nunc lacinia sollicitudin ante, sit amet tristique tortor consequat nec. Donec ut cursus libero. Etiam at massa condimentum, condimentum lacus nec, varius arcu. Suspendisse vitae nulla non odio ullamcorper molestie. Aliquam erat dolor, fermentum vel condimentum nec, porttitor ut mauris. Praesent vitae diam at mi accumsan ultrices a ut purus. Pellentesque tristique accumsan elit, non rutrum metus egestas sed.

 

[gallery size="full" ids="35736,35725,35726"]

[gallery size="full" ids="35741,35742,35744"]

 

[button type="primary" url="#"]Custom shortcode![/button]`

@Sidsector9
Copy link
Member Author

@mdesplenter thanks for sharing the details. This is working for me, let me know if I tested it incorrectly.
Also just to confirm, did you run the build step?

I have tested it with 2 posts similar to the format you provided. Please watch the video below:

Screen-2024-06-28-11807.AM.mp4

@dsawardekar
Copy link
Collaborator

@Sidsector9 I'm getting similar stalling on a larger database. I think waiting for promises is going to be always be tricky because legacy content can be diverse. We do not have any control over the block type mix in the post_content. Similarly mutation may or may not happen too.

My suggestion here is to update this as,

  1. Refactor the BE to return a init_delay in the localized vars. (https://github.com/10up/convert-to-blocks/blob/develop/includes/ConvertToBlocks/MigrationAgent.php#L31)
  2. Filter this, with apply_filters( 'convert_to_blocks_init_delay', 500, $post_id ) , so that users can override it per their legacy content needs. Default to 500 ms, so that existing projects are unaffected.
  3. On the client-side, alter the init code to use the init_delay. (https://github.com/10up/convert-to-blocks/blob/develop/assets/js/editor/editor.js#L72)

Based on how testing goes, we can then update the documentation to suggest a recommended init_delay for different types of legacy content.

@mdesplenter
Copy link

Note Deactivating all my other active plugins does help to trigger the save action action again. The main plugin that is bugging/breaking during loading is the Yoast SEO plugin, it's adding UI screens while the converter already started. I don't think you need it when converting to blocks, so I would add to the FAQ that users should disable as much plugins as possible before converting.

But still have the first problem with the images in galleries not getting converted in time. The init delay sounds good for my use case, will try that when it's here to test.

@Sidsector9
Copy link
Member Author

Sidsector9 commented Jun 28, 2024

I wanted to point out the init_delay here won't work.

It's not the lack of delay in beginning the transform process that is causing the problem. The issue is because the editor is saving posts with static blocks before the blocks have finished DOM updates.

If it is delay that we are going forward with, then we should delay client.save() as suggested by @mdesplenter in the issue.

I have opened an alternative PR #173

@jeffpaul @dsawardekar suggested we keep this PR open and instead open a new PR with an alternative approach as they will be revisiting this issue next week.

@jeffpaul
Copy link
Member

jeffpaul commented Jul 5, 2024

@Sidsector9 with #173 merged in, we good to close this PR?

@Sidsector9 Sidsector9 closed this Jul 5, 2024
@jeffpaul jeffpaul removed this from the 1.4.0 milestone Jul 8, 2024
@jeffpaul jeffpaul deleted the fix/170 branch July 9, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save is executed too fast for gallery
5 participants