-
Notifications
You must be signed in to change notification settings - Fork 220
Add the Interactivity API runtime to WooCommerce blocks #8220
Conversation
A plugin for optional chaining is required as the repo uses Webpack 4 for now.
$should_enqueue = apply_filters( '__experimental_woocommerce_blocks_enqueue_directives_runtime', false ); | ||
if ( ! $should_enqueue ) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the filter or the enqueue because wp_enqueue_script( 'wp-directives-runtime' )
should always be called as a dependency of another script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just saw these comments after commenting ourselves on the enqueueing. Your proposal for removal of the filter makes sense, so sorry @DAreRodz for the confusion. We removed the redundant comments 😅
However, as of the removal of these, the testing instructions for this PR are not correct anymore. So we should update them so that testers can try out these specific changes 😊
true | ||
); | ||
|
||
wp_enqueue_script( 'wp-directives-runtime' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of this too.
tsconfig.base.json
Outdated
@@ -39,6 +39,8 @@ | |||
], | |||
"@woocommerce/base-hocs/*": [ "assets/js/base/hocs/*" ], | |||
"@woocommerce/base-hooks": [ "assets/js/base/hooks" ], | |||
"@woocommerce/base-interactivity": [ "assets/js/base/interactivity" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@woocommerce/base-interactivity": [ "assets/js/base/interactivity" ], | |
"@woocommerce/interactivity": [ "assets/js/base/interactivity" ], |
What do you think if we rename it to @woocommerce/interactivity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion. 👍
presets: [ | ||
[ | ||
'@babel/preset-react', | ||
{ | ||
runtime: 'automatic', | ||
importSource: 'preact', | ||
}, | ||
], | ||
], | ||
plugins: [ | ||
'@babel/plugin-proposal-optional-chaining', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presets: [ | |
[ | |
'@babel/preset-react', | |
{ | |
runtime: 'automatic', | |
importSource: 'preact', | |
}, | |
], | |
], | |
plugins: [ | |
'@babel/plugin-proposal-optional-chaining', | |
], | |
presets: [ | |
[ | |
'@wordpress/babel-preset-default', | |
{ | |
modules: false, | |
targets: { | |
browsers: [ | |
'extends @wordpress/browserslist-config', | |
], | |
}, | |
}, | |
], | |
[ | |
'@babel/preset-react', | |
{ | |
runtime: 'automatic', | |
importSource: 'preact', | |
}, | |
], | |
], |
It seems that it is not necessary to install and use @babel/plugin-proposal-optional-chaining
. We can use @wordpress/babel-preset-default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say that I already tried, and @wordpress/babel-preset-default
was messing up with Preact, adding @wordpress/element
instead, so I opted to use that plugin instead of the preset you mentioned.
This is the part that replaces createElement
with @wordpress/element
:
https://github.com/WordPress/gutenberg/blob/trunk/packages/babel-preset-default/index.js#L80-L82
Great job! It looks amazing and makes us super excited! @gigitux and me were testing the functionality and there seems to be a bug with the implementation of the router. Relative URLs are apparently a problem when fetching. You can see this in the Pagination component if, instead of clicking on “Next Page” you click on the numbers for the page on a Product Query block set without the option “Inherit query from template”. For some reason (which should probably be addressed in Gutenberg), the pagination component renders a relative URL in the Is this a known issue? Should the interactivity only work with absolute URLs? |
Hey @sunyatasattva, thank you very much for your review. 😄 Glad you discovered that bug; we were not aware of it. 😅 It should be a quick fix, though, so I'll open a PR right away at https://github.com/WordPress/block-hydration-experiments/ and update the code here with the changes. |
@sunyatasattva, @gigitux, this should be ready. ☝️ |
The release ZIP for this PR is accessible via:
Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/interactivity/components.js
assets/js/interactivity/directives.js assets/js/interactivity/hooks.js assets/js/interactivity/router.js assets/js/interactivity/utils.js assets/js/interactivity/vdom.js assets/js/interactivity/wpx.js |
Size Change: +10.3 kB (+1%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise it looks fine to me. We have to rename everything to woo-
, but we can do that in a different PR.
I'm going to approve it, but please wait until someone from Woo has also approved it to merge it.
Also, @DAreRodz:
|
Thanks for updating the PR. Tomorrow I will take a look!
|
I've just run locally I'm restarting both actions just in case. |
Oh, I just saw your comment, @gigitux! 😄 Thanks for the clarification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for your work! Amazing 🚀
I added the label skip-changelog
, and edited the description adding a checkbox about the testing instructions. This way, the release manager will know that he doesn't add this PR in the testing instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
Thanks, folks! 😊 I think I don't have permission to merge this PR apart from enabling auto-merge―which doesn't work because there are some failing checks. It would be great if you could merge and close this PR for me. 🙏 |
@DAreRodz, thanks for your work! Merged! |
What?
Includes the current WP directives runtime inside the WooCommerce blocks repository and makes it ready to use.
Tracking issue: woocommerce/woocommerce#42486
Why?
This is required to start testing the Interactivity API in WooCommerce blocks until it is natively available in WordPress.
How?
false
by default).Testing instructions
See #8026 (comment)