All Products block: Try hydrating #21
Conversation
The following is a bit hacky. If we stick with this techn...The following is a bit hacky. If we stick with this technique, we might want to change apply_block_supports() to accepts a block as its argument.
woocommerce-blocks-hydration-experiments/src/BlockTypesController.php Lines 162 to 175 in d7204a8 🚀 This comment was generated by the automations bot based on a
|
Guard against unset.Guard against unset.
woocommerce-blocks-hydration-experiments/src/BlockTypesController.php Lines 148 to 159 in d7204a8 🚀 This comment was generated by the automations bot based on a
|
|
Size Change: -99 B (0%) Total Size: 878 kB
ℹ️ View Unchanged
|
|
FWIW, I spun off a PR against the upstream repo to carry over the migration of the All Products block to |
ockham
changed the title
ockham
force-pushed
the
try/hydrating-all-products-block
branch
2 times, most recently
from
31ec78d to
fe64a58
Compare
ockham
force-pushed
the
try/hydrating-all-products-block
branch
from
fe64a58 to
d77049b
Compare
Add cases for other source types.Add cases for other source types. 🚀 This comment was generated by the automations bot based on a
|
|
@michalczaplinski I've merged your branch from #39 into this one and applied some minor changes (23b0f51) to use 'our' hydration technique (from the Block Hydration Experiments repo). I think it's basically working. Hooray? 🎉 😄 (I should say that it's not a very big feat since the it's not that different from the hydration technique that was previously used by the block.) (Edit: Currently getting a block validation error in the editor -- TBH I didn't care much for backcompat and proper deprecations for the sake of this experiment 😬 ) |
I think this is because of the changes to the I see that some e2e tests are also failing but I think that they are failing for all PRs so this is probably unrelated. I ll try to pull the latest changes from upstream Also, I have manually disabled (from the github UI) the |
|
In any case, it does indeed seem to work just fine 👏 😄 Nice work @ockham Are you planning to add more changes before we can merge this? |
Oh yeah, that's certainly the reason -- what I meant to say is that I haven't bothered to write a block deprecation to handle this just yet. I might do that though so we can merge the PR without sacrificing backwards compatibility -- should be fairly straight-forward 😄
Thank you! AFAICS, they've been failing consistently in the upstream repo for a while 😬 (They sure were when I filed my
👍 |
Thank you! 😊
I'll add that block deprecation I mentioned in my other comment 😄 |
|
I now get every now and then on the frontend. Looks like some race condition to me, probably related to |
I have seen it one time as well when I ran your PR locally for the first time. But I restarted webpack and Could it be that the |
|
I can confirm that there is indeed a race condition: 2022-08-02_11-50-56.mp4Whenever the Also, you can just put a undefined but if there's no error it will log the Comp
So, in conclusion, we have to ensure that the block registration is always run before the hydration 🙂. |
Thanks a lot for confirming!
Fun fact, I had a Now on my new M1 Pro, I get |
|
So, I see two ways of handling this:
diff --git a/assets/js/base/utils/bhe-frontend.tsx b/assets/js/base/utils/bhe-frontend.tsx
index 318f02ce4..ba122be7f 100644
--- a/assets/js/base/utils/bhe-frontend.tsx
+++ b/assets/js/base/utils/bhe-frontend.tsx
@@ -72,7 +72,10 @@ Children.shouldComponentUpdate = () => false;
class WpBlock extends HTMLElement {
connectedCallback() {
- setTimeout( () => {
+ setTimeout( async () => {
+ const blockType = this.getAttribute( 'data-wp-block-type' );
+ await whenRegistered( blockType );
+
// ping the parent for the context
const event = new CustomEvent( 'wp-block-context', {
detail: {},The caveat is that we might have to add a timeout of sorts in case the block fails to register so that this
What do you think @ockham ? 🙂 |
|
Thanks a lot for your investigation @michalczaplinski -- much appreciated! ❤️ When I was thinking about the issue earlier, I was also wondering if manually introducing some asynchronicity could solve the problem. TBH I didn’t quite see how we can create a promise during block registration that the custom element listens to 🙈 but I’m super curious to see what solution you come up with! 😄 I think I was personally leaning more towards solution #2. IIRC, the block’s |
I looked it up: So in BHE, we're using the I haven't totally tracked it down in Woo Blocks, but I've found at least some related Webpack logic and documentation. It seems like those frontend scripts get enqueued via |
- The registry is now the instance of a BlockRegistry class. - The BlockRegistry class has a `whenDefined()` method which is used to await the registration of a block with a given name.
|
Thanks for digging a bit deeper @ockham ! We took a look with Carlos and this is what we found:
I can't couldn't think of another way to solve this problem today so I went ahead and wrapped the block registry in a class that implements a It solves the bug and I think the code is not all that terrible 😅 I can also put it a separate PR! |
Indeed: Woo seems to have an allowlist of scripts to look out for in a given block directory, and only those files will be built by Webpack. (Here's the code for
Great sleuthing! 👍 🕵️
Impressive, thanks for that solution! |
|
Being somewhat of a die-hard minimalist (occam's razor and all 😉), I had another look at the problem, based on your findings above 😬 Essentially, However, the same is true of the corresponding file in BHE, so apparently, simply importing What would seem more problematic is if Well, turns out we're kinda doing that: https://github.com/woocommerce/woocommerce-blocks-hydration-experiments/pull/21/files#diff-b1f2eacd00a094cfdf9ca09fbfb8ea734306805ada4af1ef6042bec87c617405R10 😬 That
Looks like 😬 For this PR, we're only importing diff --git a/assets/js/base/utils/index.js b/assets/js/base/utils/index.js
index 5f1aa8fd1..802954efe 100644
--- a/assets/js/base/utils/index.js
+++ b/assets/js/base/utils/index.js
@@ -7,5 +7,5 @@ export * from './get-valid-block-attributes';
export * from './product-data';
export * from './derive-selected-shipping-rates';
export * from './get-icons-from-payment-methods';
-export * from './bhe-frontend';
+//export * from './bhe-frontend';
export * from './bhe-element';
Better 😄 If this solution seems fragile (loading order and all), I'm pretty sure we can make it solid: We can isolate the |
|
Awesome work @ockham ! 👏 I had a lingering suspicion that this should be solved by being careful about importing modules but I didn't see how to approach it, major kudos! 🙂 With that out of the way, I think we can merge it now 🙂 |
|
Sorry for being a bit late to the party, but this race condition was expected, and it is what I meant with the Make sure Frontend components are automatically hydrated even if their component is registered after the connectedCallback execution task in the BHE Tracking issue. I'll open an issue in the BHE repository. |
WIP. Some early experiments, applying techniques from https://github.com/WordPress/block-hydration-experiments to the All Products block. See #20 for a vague roadmap.
To test, insert the "All Products" block into a page of your choice. View it on the frontend, and verify that it renders correctly and reacts to user interaction.
TODO:
data-gutenberg-attributes). Find out why, fix.