search

Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

WooCommerce Blocks: Adding the Cart block crashes the editor #57449

Closed AtrumGeost closed 2 years ago

AtrumGeost commented 3 years ago

Update from @JoshuaGoode

Please see internal notes p9F6qB-7LA-p2#comment-43978

Related 304-gh-Automattic/wpcomsh

34655

Quick summary

When adding the WooCommerce Cart block I get this error:

TypeError: Cannot read properties of null (reading 'removeChild')
    at h.componentWillUnmount (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/amp/assets/js/amp-block-editor.js?ver=2ee98517d90b7670631d2afc67df7285:31:1273)
    at Rh (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:148:43)
    at Sh (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:151:394)
    at Qj (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:174:181)
    at unstable_runWithPriority (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react.min.755b0ae9.js:24:26)
    at Za (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:73:8)
    at eb (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:170:163)
    at gf (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:162:317)
    at https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react-dom.min.edd8b7a6.js:73:230
    at unstable_runWithPriority (https://woocrashcourse101463901178.wpcomstaging.com/wp-content/plugins/gutenberg/vendor/react.min.755b0ae9.js:24:26)

Steps to reproduce

  1. On an AT Site install WooCommerce blocks: https://woocommerce.com/products/woocommerce-gutenberg-products-block/
  2. Create a new page and add the WooCommerce Cart block
  3. Notice the error

I confirmed this doesn't happen on a WordPress.org site (Jurassic Ninja)

What you expected to happen

For the editor to display the Cart block

What actually happened

Got this message:

The editor has encountered an unexpected error.

https://user-images.githubusercontent.com/3696121/139307197-8a8c846b-6e78-4004-ae71-a7c815ebaf47.mov

Console error:

Screen Shot 2021-10-28 at 12 41 13

Context

I encountered this while doing the WooCommerce Crash Course

Operating System

macOS

Browser

Google Chrome/Chromium

Simple, Atomic or both?

Atomic

Theme-specific issue?

No response

Other notes

No response

Reproducibility

Consistent

Severity

Some (< 50%)

(WooCommerce Blocks are an experimental feature )

Available workarounds?

No response

Workaround details

No response

Robertght commented 3 years ago

Thank you for reporting this @AtrumGeost !

For me, it just freezes the entire editor/page.

Robertght commented 3 years ago

@pmcpinto should we move this one to https://github.com/Automattic/wc-calypso-bridge ?

pmcpinto commented 3 years ago

@pmcpinto should we move this one to https://github.com/Automattic/wc-calypso-bridge ?

Thanks for the ping. I think it makes more sense to transfer to the https://github.com/woocommerce/woocommerce-gutenberg-products-block repo. @nerrad what do you think?

Gustavo-Hilario commented 3 years ago

I'm taking the Woo Crash Course, and I'm also reproducing a similar issue while trying to add the Checkout block to one of my pages.

Unfortunately, the editor froze completely and I couldn't get a console report. I have tried clearing my Chrome cache, using an incognito window, and also using the WordPress.com desktop app, but the issue persists.

nerrad commented 3 years ago

It looks like this may be specific to the Atomic environment but ya it can be transferred to Woo Blocks repo.

nerrad commented 3 years ago

Although, there's also some possibility that it could be conflicting with something Calypso adds in the Atomic environment? Let's leave it here for now and I've asked @woocommerce/rubik-fp-squad to look into it.

nerrad commented 3 years ago

Just a quick update. Inserting the Cart block in the atomic environment is setting off an infinite loop of Block SSR requests which is what is triggering the breakage.

Requests are to this endpoint:

/wp-json/wp/v2/block-renderer/woocommerce/product-new?

block-renderer is the endpoint used by the ServerSideRender component. We're investigating why it only happens in Atomic and not on self-hosted (or even ephemeral).

dpasque commented 3 years ago

Tagging @Automattic/flow-patrol-create based on issue content

nerrad commented 3 years ago

So it looks like the differentiating factor here is this is reproducible in the iframed Block Editor (within the Calypso environment). I'm guessing that what's happening is the unique block structure of the Cart block (with different view contexts) somehow causes it to be mounted and unmounted multiple times which might be triggering the multiple requests from the ServerSideRendered component in the one view.

senadir commented 3 years ago

👋🏼 I managed to find the code causing the issue:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/assets/js/blocks/cart-checkout/shared/use-forced-layout.ts#L94-L99

A background on Cart block:

so synchronizeBlocksWithTemplate along with replaceInnerBlocks are causing this loop.

nerrad commented 3 years ago

I was able to reproduce this on an ephemeral site. It looks like I've isolated this to a conflict with Jetpack (which I was unable to deactivate on Atomic). When I deactivate Jetpack (fully connected, and on free plan), I'm unable to reproduce the error.

senadir commented 3 years ago

Hey again.

We managed to limit this bug to the jetpack version included in wordpress.com and the public version of jetpack, as of writing 10.2.1 Updating to 10.3-beta fixed the issue so this should be fixed by them.

nerrad commented 3 years ago

@Automattic/jetpack-crew for your awareness. We did check the changelog and didn't see anything obvious around what might explain the issues but it might be good to have on your radar in case something similar happens in the future. What we saw was there was some sort of race condition kicked off with the way the Cart block was rendered with Jetpack active (the broken version). Please do connect with @senadir if you want any additional info around the structure of the Cart block.

kraftbj commented 3 years ago

Hey again.

We managed to limit this bug to the jetpack version included in wordpress.com and the public version of jetpack, as of writing 10.2.1 Updating to 10.3-beta fixed the issue so this should be fixed by them.

To confirm, the version of Jetpack on Atomic? Just want to confirm we're not talking about WordPress.com Simple.

You can confirm the version number of Jetpack on the Atomic site via Tools>Site Health>Info>Jetpack.

What's concerning me is that Atomic should be on 10.3-beta already.

kraftbj commented 3 years ago

Will also ping in @Automattic/manage since this is exclusively happening on Atomic via the iframe editor (if I'm reading the history right).

nerrad commented 3 years ago

What's concerning me is that Atomic should be on 10.3-beta already.

Hmm, ya so that is concerning. Here's the latest testing (I deactivated all Jetpack modules for each site to try to eliminate some complexity):

Atomic Site (https://nerradssuperstore.wpcomstaging.com/).

On Ephemeral Site (https://ephemeral-senadir-20211029.atomicsites.blog/)

On Ephemeral Site (https://ephemeral-nerrad-20211029.atomicsites.blog/)

Summary.

Edit: I forgot about using Site Health to get specific version numbers so I updated the above with them.

drwpcom commented 3 years ago

Making a note of 4418000-zen, for us to notify the user when the issue is resolved.

samiff commented 3 years ago

It looks like we have different versions of 10.3-beta on working/non-working sites which may help isolate what introduced the breakage.

Just noting that when we save the version number to jetpack_options in the local db, we are concatenating a unix timestamp as seen here. This option value is what is displayed on the Site Health screen. So 10.3-beta:1635516828 and 10.3-beta:1635591756 should be referring to the same Jetpack 10.3-beta version.

nerrad commented 3 years ago

This option value is what is displayed on the Site Health screen. So 10.3-beta:1635516828 and 10.3-beta:1635591756 should be referring to the same Jetpack 10.3-beta version.

I see, it's really odd then that on the one site it works 🤔. However, I definitely am able to verify that deactivating Jetpack on the ephemeral site where the breakage happens is fixing things.

JoshuaGoode commented 2 years ago

This popped up in an internal report.

p9F6qB-7LA-p2

JoshuaGoode commented 2 years ago

Testing further, this requires an Atomic site with wpcomsh. All WPCOM Atomic sites get that but it's optional on Ephemeral and not present on other types of Atomic sites.

So, wpcomsh + Jetpack is required. See some additional notes in the comments on p9F6qB-7LA-p2

JoshuaGoode commented 2 years ago

Confirmed wpcomsh is contributing here.

See internal notes p9F6qB-7LA-p2#comment-43978

Related 304-gh-Automattic/wpcomsh https://github.com/Automattic/wp-calypso/pull/34655

sharonlaker19 commented 2 years ago

+1 at 33033942-hc

Nic-Sevic commented 2 years ago

another instance here: 33060339-hc still happening with coblocks disabled which matches Joshua's findings

openfist commented 2 years ago

This issue has returned 33312461-hc.

sharonlaker19 commented 2 years ago

Another report at 4649687-zen

Robertght commented 2 years ago

another case in 4751893-zen

kaushikasomaiya commented 2 years ago

Another report: 4758877-zen

ClassicRKR27 commented 2 years ago

Another instance here: 34725540-hc

melek commented 2 years ago

User @ClassicRKR27 referred to in 34725540-hc returned, now has ticket at 4883934-zen for updates.

jeherve commented 2 years ago

I was able to narrow the problem down to the WordPress.com block editor functionality (developed here).

The script is loaded via the Jetpack plugin, on all WoA sites that are connected to WordPress.com, here: https://github.com/Automattic/jetpack/blob/4a9a5c19ffa1ad972c6e4adbf08e04173d8401b1/projects/plugins/jetpack/modules/wpcom-block-editor/class-jetpack-wpcom-block-editor.php#L349-L365

To bypass the issue, one can opt to not load the WordPress.com block editor functionality at all, like so:

add_filter( 'jetpack_tools_to_include', function( $tools ) {
    $index = array_search( 'wpcom-block-editor/class-jetpack-wpcom-block-editor.php', $tools );
    if ( $index ) {
        unset( $tools[ $index ] );
    }
    return $tools;
} );

(You'll want to add that code snippet to your WoA site, via a functionality plugin for example)

To set up an enviroment where you can play and debug the problem, try the following:

I found that when removing both trackBlockInsertion and trackInnerBlocksReplacement below, the problem disappeared: https://github.com/Automattic/wp-calypso/blob/7adfde3c9b66198df497ece0820dd537efb31a82/apps/wpcom-block-editor/src/wpcom/features/tracking.js#L723-L727

Related PRs:

34655

39396

Adding a new exception, like what was done in #53501, but this time for the Cart block, does not work since the Cart block has different inner blocks:

diff --git a/apps/wpcom-block-editor/src/wpcom/features/tracking.js b/apps/wpcom-block-editor/src/wpcom/features/tracking.js
index b286a17be7..3cc2d28d45 100644
--- a/apps/wpcom-block-editor/src/wpcom/features/tracking.js
+++ b/apps/wpcom-block-editor/src/wpcom/features/tracking.js
@@ -394,7 +394,9 @@ const trackInnerBlocksReplacement = ( rootClientId, blocks ) => {
            // Reusable Block
            name === 'core/block' ||
            // Post Content
-           name === 'core/post-content'
+           name === 'core/post-content' ||
+           // Woo's Cart block
+           name === 'woocommerce/cart'
        ) {
            return;
        }

cc @senadir

Addison-Stavlo commented 2 years ago

Adding a new exception does not work since the Cart block has different inner blocks:

I don't follow, all of these blocks in the exception have different inner blocks. Why would specific inner block content matter for an exception that only considers the parent block's name for an early return?

senadir commented 2 years ago

Cart has 4 levels deep of inner blocks, all of those levels call replaceInnerBlocks by themselves, so unless the check traverse all the way up to the parent block (which it doesn't it only check the most immediate parent), you will need to check for all of those parents woocommerce/cart, woocommerce/filled-cart, woocommerce/empty-cart, woocommerce/cart-order-summary, woocommerce/cart-items just in the cart block, more to come in the future and this doesn't include Checkout block. So if you want to bail out early, you will need to keep traversing until you find woocommerce/cart, which might fall at odds with the current logic.

Addison-Stavlo commented 2 years ago

Adding a new exception does not work since the Cart block has different inner blocks:

you will need to check for all of those parents woocommerce/cart, woocommerce/filled-cart, woocommerce/empty-cart, woocommerce/cart-order-summary, woocommerce/cart-items just in the cart block

I think the WHY is incorrect here. After digging further it seems this errors before that early return check. Simply having ANY handler function here () => {} seems to trigger this error on inserting the Cart block, while removing all handlers from insertBlock(s) and replaceInnerBlocks seems to resolve the issue 🤔 . Neither of these seem to be async actions either so im unsure about the calling async actions sync problem theory.

Cart has 4 levels deep of inner blocks, all of those levels call replaceInnerBlocks ...

If that is the case it sounds like this block is very heavy on calling actions when it is inserted. Probably much more heavy than any block we have dealt with up until this point. Even noop replacements of the tracking handlers on these actions seems to push it over the edge and cause Maximum update depth exceeded. I wonder if there is a more graceful way for this block (and its corresponding children) to be inserted. 🤔

JessBoctor commented 2 years ago

I had another report of this issue in #35059800-HC.

arinoch commented 2 years ago

User in 5175251-zd-woothemes has the issue referred to earlier re: the WooCommerce checkout block freezing the editor. The shortcode works without freezing, so provided that as a workaround.

thesacredpath commented 2 years ago

Any word on when this might get addressed? Have a user who has been waiting for a good while.

5175251-zen

kraftbj commented 2 years ago

@senadir @jeherve Pinging y'all from the proposed solution in https://github.com/Automattic/wp-calypso/pull/62517.

It sounds, from that discussion, that the proposed solution in 62517 is not viable. If that's the case, can we close it?

Is there a person/team tracking this in order to bring it to resolution? It's a bit out of Jetpack's wheelhouse so I'd hope a WP.com team can take it, even though @jeherve laudably offered an initial solution.

jeherve commented 2 years ago

I believe @Addison-Stavlo had started taking a look, as discussed in p1649160607859449/1649145116.774249-slack-CDLH4C1UZ , but ran into issues as mentioned in #62517.

tarunvijwani commented 2 years ago

On a WordPress.com site, I was able to add the Cart and Checkout blocks to a page. But the front end doesn't load the content of the blocks. In the editor, it loads just fine.

Site: https://testingatomicsite.wpcomstaging.com/

WooCommerce: 6.6.1 WooCommerce blocks: 8.0.0

Editor Front-end
image image
Addison-Stavlo commented 2 years ago

@senadir @jeherve Pinging y'all from the proposed solution in https://github.com/Automattic/wp-calypso/pull/62517. It sounds, from that discussion, that the proposed solution in 62517 is not viable. If that's the case, can we close it?

Regarding this bug, that PR was a red herring. The changes there are suggested as an improvement to the tracking system, but in testing make no functional difference (with this specific bug nor with async actions). We can close it or merge it (i just closed it for now), but it really doesn't seem to make any difference.

Regarding the tracking system and the Cart block - IIRC from our investigations the issue was not with dotcom tracking, but with the way the cart block itself was set up problematically (will have to dig more to remember context, but something like a lot of nested inner block controllers and compounding sync calls causing unrealistic amounts of insertBlocks and replaceInnerBlocks calls) and that folks had starting digging into how to fix the underlying issue in the Cart block itself.

IIRC Tracking is just the straw that breaks the camels back in this circumstance, the match that ignites the gas leak. The only thing we could do was remove the match entirely - IE remove any and all tracking related to insertBlocks and replaceInnerBlocks (which seem pretty important to dotcom), even replacing the related tracking handlers with noop functions continues to trigger the cart block to overload, and only when they are removed entirely does it start to work again. This issue should be resolved at the source of the leak.

senadir commented 2 years ago

I'm going to be digging into this issue again because we identified as a blocker for enabling Cart/Checkout blocks in WooCommerce core. I'm still positive that something in the tracking system is causing this and it's not just how inner blocks works because we didn't see any performance issues nor this bug anywhere else except wpcom.

github-actions[bot] commented 2 years ago

Support References

This comment is automatically generated. Please do not edit it.

senadir commented 2 years ago

This is issue is now fixed, but we couldn't fully fix the source of it, so it might surface again. https://github.com/woocommerce/woocommerce-blocks/pull/6718 has a good explanation of why this happens and how it might surface again. TL;DR is surface from a genuinely safe code, basically using dispatch inside useSelect would cause this. We didn't fully understand why this happens, but we refactored our useSelect calls to avoid using dispatch. Since we can't enforce a linter rule to prevent that (nothing I can think of), it might surface again due to someone reintroducing this pattern and it passing reviews.