-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Script Modules: Move data passing to 6.7 compat file #64006
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
You can see an example with latest trunk where this results in script data being printed twice: document.querySelectorAll('[type="application/json"]');
// NodeList(2) [script#wp-script-module-data-@wordpress/interactivity, script#wp-script-module-data-@wordpress/interactivity] (Notice two scripts with the same ID are printed). |
add_action( | ||
'after_setup_theme', |
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.
Core uses after_setup_theme
so we need to wait for that before the has_action
checks will work correctly.
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.
Should the priority
parameter be used to ensure it runs later?
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.
Another thing I was wondering is if it would be easier to check that the new method introduced in the core ticket exists. Something like this:
if ( ! method_exists( 'WP_Interactivity_API', 'filter_script_module_interactivity_data' ) ) {
add_action( 'wp_footer', 'gutenberg_print_script_module_data' );
add_action( 'admin_print_footer_scripts', 'gutenberg_print_script_module_data' );
}
I have neither the context nor a strong opinion, just sharing in case it is considered.
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 idea, although I believe this is working correctly and I don't think it's too important exactly how we do the feature detection.
I have added a priority, that's certainly a good idea.
This is from https://core.trac.wordpress.org/ticket/61512, in WordPress 6.7. I think that means it should be part of 6.7 compat, so I'll need to adjust that. |
The feature is slated for WordPress 6.7 (not 6.6).
3b79e4f
to
26a323a
Compare
I've rebased and moved this to 6.7 compat. |
I've been testing it, and I am not sure I understand what the compatibility code is supposed to do 😄 I've been testing this in the <script type="application/json" id="wp-interactivity-data">...</script> I can see in Additionally, I can see the name is different. Is that expected to change with the compat code as well? |
* | ||
* @param array $data The data that should be associated with the array. | ||
*/ | ||
$data = apply_filters( "script_module_data_{$module_id}", array() ); |
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.
Taking a deeper look, I believe this is always returning an empty array because this code in core was added in 6.7, so it is not in 6.6.
That means that it is never entering the next conditional and it doesn't run wp_print_inline_script_tag
.
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.
If you use recent Gutenberg and Core trunk, you'd see an identical script tag printed twice (once by Core and once by Gutenberg). The fact that that isn't happening is a good sign 🙂
To test pre-6.7, you really need to add code which makes it a but uncomfortable to test. If the interactivity API is in use, you could add a filter like this:
add_filter( "script_module_data_@wordpress/interactivity", function ( $data ) {
$data['foo'] = 'bar';
return $data;
} );
You should see both script tags printed (id="wp-interactivity-data"
and id="wp-script-module-data…
). Note that if you do this, Interactivity API won't have server-provided data because it prefers the filtered script tag which won't have the correct data. That's expected and fine, this is only for some manual testing.
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.
If you use recent Gutenberg and Core trunk, you'd see an identical script tag printed twice (once by Core and once by Gutenberg).
Yes, this part is working fine in my testing 🙂
Okay, so let me check if I am understanding it correctly:
- In WordPress 6.7, we are adding the
apply_filters( "script_module_data_{$module_id}", array() )
for first time: link. - That means that until 6.7, no developers should have used the
add_filter( "script_module_data_..." )
you shared in the example. - This compatibility code only runs when an external developer uses the
add_filter( "script_module_data_..." )
.
Is that right? If that's the case, my main question is: Is the compat code really necessary?
(Sorry if it is obvious, I am still trying to fully understand the use case 😄 )
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.
Close, but this was introduced in Gutenberg first, so this isn't entirely correct:
That means that until 6.7, no developers should have used the add_filter( "script_module_data_..." ) you shared in the example.
Is the compat code really necessary?
I would not add this compat code if it were not already in Gutenberg. But because the code is in Gutenberg, the pre-existing Gutenberg implementation now becomes compat code for WordPress 6.7.
Existing code of a new (experimental) feature in Gutenberg has transformed to compatibility code for WordPress 6.7.
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.
Thanks for the context 🙂 I assumed it was introduced as an experiment, so it could be considered as "not added in Gutenberg."
Should be a more robust way of ensuring that the plugin's filter function executes _after_ Core's.
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.
Testing looks good on my end 🙂 As shared here, I am still not 100% sure if the compat code is needed at all, but you have much more context than me. It is definitely an improvement from trunk
because the script is not duplicated.
I think it's good to keep this as "compat" even if that may not be strictly accurate. If it were removed, we'd effectively have a regression in a Gutenberg feature until Core is released with support for the feature. Keeping it in the compatibility layer makes it clear that it's required until support is dropped. |
Move Script Modules data passing to compatibility directory. Add conditions to ensure the functionality is duplicated when present in Core. This prevents duplicate script tags with duplicate IDs from being printed. This feature is planned to ship in WordPress 6.7: https://core.trac.wordpress.org/ticket/61510
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
What?
6.66.7 compat file. This feature is on trunk and slated for WordPress 6.7.Why?
This feature landed in core and is planned for release in 6.7. It is not experimental, but part of the WordPress <6.7 compatibility layer.
The filters for printing script data should be registered conditionally to prevent duplicating Core behavior and printing that data multiple times.
Testing
Try using this Gutenberg plugin locally with script data. The current WordPress trunk implementation of Interactivity API uses script module data (changed here) to enqueue Interactivity data. Any stores with server side data should use this mechanism to pass data to the client, so it can be observed that way, like in this comment: #64006 (comment).