File: Add experimental integration with Interactivity API #50377
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gziolo
added
[Type] Enhancement
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
from
630294d
to
335e9f8
Compare
|
Size Change: -11.2 kB (-1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
from
335e9f8
to
cf365be
Compare
|
Flaky tests detected in cf365be. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4913216119
|
gziolo
commented
May 8, 2023
| @@ -39,7 +45,7 @@ function gutenberg_register_interactivity_scripts( $scripts ) { | |||
| * @return string The modified script tag. | |||
| */ | |||
| function gutenberg_interactivity_scripts_add_defer_attribute( $tag, $handle ) { | |||
| if ( str_starts_with( $handle, 'wp-interactivity-' ) ) { | |||
| if ( str_starts_with( $handle, 'wp-interactivity-' ) || str_contains( $tag, 'view/interactivity.min.js' ) ) { | |||
There was a problem hiding this comment.
Assuming that we always have the Interactivity script in view/interactivity.min.js file, we can temporarily enforce defer. Otherwise, the runtime might load after the view script.
In the future, we can handle defer in the WordPress core by detecting its dependency on the wp-interactivity-runtime.
gziolo
commented
May 8, 2023
| @@ -220,15 +220,21 @@ module.exports = [ | |||
| ].filter( Boolean ), | |||
| }, | |||
| { | |||
| ...baseConfig, | |||
There was a problem hiding this comment.
This brings some shared setting used for all other configs:
- Browserslist integration (see Add Interactivity API runtime #49994 (comment)).
- Bundle analyzer integration.
- Custom config for the terser plugin.
- Development vs production mode aligned with how
npm run buildandnpm run devwork. - Integration with devtol.
gziolo
commented
May 8, 2023
| @@ -278,5 +286,12 @@ module.exports = [ | |||
| }, | |||
| ], | |||
| }, | |||
| plugins: [ | |||
| ...plugins, | |||
There was a problem hiding this comment.
Integrates webpack plugins used with all other configs:
- Integration with bundle analyzer.
- Integration with the plugin that replaces globals like
process.env.IS_GUTENBERG_PLUGIN,process.env.IS_WORDPRESS_CORE. Once Warning: IntroduceSCRIPT_DEBUGto make the package compatible with webpack 5 #50122 lands, it would also make it possible usingSCRIPT_DEBUG- that could be a way to runconsole.logonly in development mode in a way where it's completely removed in the production build. - Custom setup for
DependencyExtractionWebpackPlugin, that together with another WP specific pluginReadableJsAssetsWebpackPluginensure that all necessary files are created in the filesystem insidebuild/block-library:
gziolo
commented
May 8, 2023
tools/webpack/blocks.js
Outdated
| entry: { | ||
| // blockname: './packages/block-library/src/blockname/interactivity.js', | ||
| file: './packages/block-library/src/file/view/interactivity.js', |
There was a problem hiding this comment.
This made me realize that interactive blocks are the only scripts that are transpiled with Babel from webpack. We have a two-step workflow for everything else to ensure that 3rd party projects can consume files from build (CJS) and build-module folders (ESM).
gziolo
commented
May 8, 2023
| 'build/block-library/interactive-blocks/vendors.min.js' | ||
| ) | ||
| ); | ||
| foreach ( array( 'vendors', 'runtime' ) as $script_name ) { |
There was a problem hiding this comment.
I wanted to explore how we could automate registering the runtime based on the asset files that we use for all other scripts built with webpack. In particular, the version generate is very important as it only changes when the file's content changes, so it's essential for proper caching in WordPress core.
22 tasks
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
from
cf365be
to
77db14b
Compare
gziolo
commented
May 16, 2023
| @@ -95,9 +95,6 @@ function gutenberg_enable_experiments() { | |||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-details-blocks', $gutenberg_experiments ) ) { | |||
| wp_add_inline_script( 'wp-block-editor', 'window.__experimentalEnableDetailsBlocks = true', 'before' ); | |||
| } | |||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-interactivity-api-navigation-block', $gutenberg_experiments ) ) { | |||
There was a problem hiding this comment.
window.__experimentalEnableNavigationBlockInteractivity is never used so can safely remove it.
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
from
77db14b
to
8939ef9
Compare
gziolo
requested review from
spacedmonkey,
ntwb,
nerrad and
ajitbohra
as code owners
It's true that as the feature itself, migrating doesn't make much sense. But the downside of not migrating this block would be that in the future, site-wide features that require all blocks to be built with the Interactivity API, like client-side navigation, would be disabled when this block is added. Regarding the size, here's how I see it:
But if a site is so simple that it has only this block, 10KB is still minimal, and the site will load quickly anyway. It won't affect the UX. It's more important to optimize for sites that have many interactive blocks. An additional API could be created for these types of blocks to "hook" into the Interactivity API, but I believe the complexity of introducing such a feature doesn't outweigh the fact that if a site only has one interactive block, we'd be optimizing something that won't benefit from such optimization. |
luisherranz
approved these changes
May 16, 2023
Comment on lines
26
to
27
| $processor->set_attribute( 'data-wp-init', 'effects.core.file.init' ); | ||
| $processor->set_attribute( 'hidden', true ); |
There was a problem hiding this comment.
It feels a bit strange to me to use a function to mutate the DOM when we have a primitive to do so. Have you considered using data-wp-bind instead? Something like this:
$processor->set_attribute( 'data-wp-bind.hidden', '!selectors.core.browserSupportsPdfs' );import { browserSupportsPdfs } from "./utils";
store({ selectors: { core: { browserSupportsPdfs } } });There was a problem hiding this comment.
Will bind run only once on the client? I wasn't sure how it would work internally.
I'm all for using bind if that is the best way to tackle it 👍🏻
There was a problem hiding this comment.
Yes, it's reactive to state and context, but it won't react (rerender) because we are not accessing them, so it will only run once.
There was a problem hiding this comment.
If I am not mistaken, it should run only once as it has no references that change. I run a quick test and that part seems to work.
However, it seems the negation operator doesn't work with selectors. My first guess is that in these functions, we are not considering properly that current can be a function. I believe this could be solved moving the hasNegationOperator logic to the getEvaluate function. Something like this:
// Resolve the path to some property of the store object.
const resolve = ( path, ctx ) => {
let current = { ...store, context: ctx };
path.split( '.' ).forEach( ( p ) => ( current = current[ p ] ) );
return current;
};
// Generate the evaluate function.
const getEvaluate =
( { ref } = {} ) =>
( path, extraArgs = {} ) => {
// If path starts with !, remove it and save a flag.
const hasNegationOperator =
path[ 0 ] === '!' && !! ( path = path.slice( 1 ) );
const value = resolve( path, extraArgs.context );
const returnValue =
typeof value === 'function'
? value( {
ref: ref.current,
...store,
...extraArgs,
} )
: value;
return hasNegationOperator ? ! returnValue : returnValue;
};I quickly tested and it seems to work, but I am not sure if it is the correct approach.
There was a problem hiding this comment.
I will wrap the function for now in the custom selector to handle negation, but we should follow-up with extending the handling for ! as it seems to important to have it working universally with both the context and selectors.
There was a problem hiding this comment.
It works correctly with 2d1c63b 🎉
There was a problem hiding this comment.
I tested that the bind never updates again with the following code:
diff --git a/lib/experimental/interactivity-api/blocks.php b/lib/experimental/interactivity-api/blocks.php
index 755c1d1d4f..8f900501f6 100644
--- a/lib/experimental/interactivity-api/blocks.php
+++ b/lib/experimental/interactivity-api/blocks.php
@@ -24,6 +24,7 @@ function gutenberg_block_core_file_add_directives_to_content( $block_content, $b
$processor->set_attribute( 'data-wp-island', '' );
$processor->next_tag( 'object' );
$processor->set_attribute( 'data-wp-bind.hidden', 'selectors.core.file.hasNoPdfPreview' );
+ $processor->set_attribute( 'data-wp-on.click', 'actions.core.file.incrementCount' );
$processor->set_attribute( 'hidden', true );
return $processor->get_updated_html();
}
diff --git a/packages/block-library/src/file/interactivity.js b/packages/block-library/src/file/interactivity.js
index cf9ae41002..76696511cc 100644
--- a/packages/block-library/src/file/interactivity.js
+++ b/packages/block-library/src/file/interactivity.js
@@ -5,13 +5,27 @@ import { store } from '../utils/interactivity';
import { browserSupportsPdfs } from './utils';
store( {
+ state: {
+ count: 0,
+ },
selectors: {
core: {
file: {
hasNoPdfPreview() {
+ console.log( 'Run!' );
return ! browserSupportsPdfs();
},
},
},
},
+ actions: {
+ core: {
+ file: {
+ incrementCount( { state } ) {
+ state.count = state.count + 1;
+ console.log( state.count );
+ },
+ },
+ },
+ },
} );This is what I see printed in the console when clicking on the PDF preview:
It made me realize that we use Preact Signals behind the scenes, which means that the selector doesn't use any signal, so it will never update as there is nothing it needs to listen to for change ...
As simple as that when you switch the mindset from React 😄
There was a problem hiding this comment.
I believe this could be solved moving the hasNegationOperator logic to the getEvaluate function.
That seems much better, yes 🙂
There was a problem hiding this comment.
I created a pull request in the block-interactivity-experiments repo to handle that. We can keep the conversation about that there.
| @@ -238,12 +245,14 @@ module.exports = [ | |||
| vendors: { | |||
| name: 'vendors', | |||
| test: /[\\/]node_modules[\\/]/, | |||
There was a problem hiding this comment.
I'd like to change this to the list of dependencies, even if we have to do it manually. Maybe in another PR :)
There was a problem hiding this comment.
If I recall correctly, @DAreRodz tried listing dependencies, but you also need to list their dependencies and so on. It might be difficult to maintain, but it's worth trying.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
SantosGuillamot
approved these changes
May 17, 2023
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
2 times, most recently
from
ef99a48
to
fdd4b5d
Compare
gziolo
force-pushed
the
update/file-block-interactivity-api
branch
from
fdd4b5d
to
2d1c63b
Compare
|
Tested in Safari on macOS: 
Tested in Safari on iOS
It works as expected 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Explore using the Interactivity API for the File block.
It's mostly a follow-up for #49994 where Interactivity API runtime landed.
I also spend some time looking at how we can scale the Interactivity API experiment so it is easier to convert more blocks using a more unified approach. Part of the effort included the refactoring, where I checked how we could generalize the experiment to all core blocks that want to use Interactivity API.
Why?
My main goal was to validate the current webpack configuration against integration with the existing core block. I also wanted to see how easy it is to convert an existing block to use Interactivity API.
How?
I’m still contemplating whether it’s a good idea to convert the File block to Interactivity API, as in practice, it isn’t needed at all… It only increases the bundle size (the size of runtime + directives), which people can use to counterargument the proposal. I see the value in migrating all view scripts to one approach, but I’d love to hear feedback from other folks.
Regarding the block, the only part I wasn't sure about was whether we should hide the PDF preview initially and only enable it when we ensure that the browser can support it. It's the opposite of what is in the
trunktoday, but I also experienced some issues with that approach when I was enforcing Safari to hide theobjectwhile it started loading the PDF document. It's hard to tell whether it's an actual issue on mobile devices that don't support PDF preview.I explored a few changes to the webpack config:
Testing Instructions
Screenshots or screencast
Show inline embed setting in the editor:
Webpack warning addressed in this PR:
JavaScript size before this PR:
JavaScript size after this PR
The size for the File block's view script is nearly the same, but we have to load the Interactivity runtime, which is around 11 kB.