-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: Add server to client data passing #6682
Script Modules: Add server to client data passing #6682
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @rio221z. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
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 left a couple of nitpicks, but not blockers.
* | ||
* @since 6.6.0 | ||
* | ||
* @param array $data The data that should be associated with the 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.
Why must the data be an array? Couldn't it be any JSON-serializable data? What if null
is passed instead as the default value, and any value other than null
will get a JSON script
printed?
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.
This question came up on the Gutenberg PR here: WordPress/gutenberg#61658 (comment)
This should be an array because it makes the interface for filters clear. If modules start to use other types then the filter interface becomes less clear. They receive and return an array: filter_func( array $data ): array
.
It's true that this data is tightly coupled to the module and its needs, so if it really just needs a string then why not let it pass that? I still think it's a good practice because as that module evolves and requires more or different data it doesn't need to change types and start dealing with an object instead of a string. This could also allow versioned data to be passed or for different versions of modules to access only the data they need.
* The data can be read on the client with a pattern like this: | ||
* | ||
* @example | ||
* const dataContainer = document.getElementById( 'wp-scriptmodule-data_MyScriptModuleID' ); |
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.
Quoting @gziolo on #6683 (comment):
See the comment in the proposal that was touching on this particular example:
I understand the benefits here, but would love to see a better initialization story. If I’m understanding correctly, any module that wants to access data will need some version of the following with only their data script handle being replaced:
[the same code snippet]
Is there any way we could simplify data access and abstract this to reduce the amount of boilerplate required for each module that wants to access server data?
I agree, but I see implementing helpers to access the data as a next step after this work is settled. To start, the data can be accessed like in this example. I also think the example could be removed and data access can be left for folks to figure out.
This PR is getting the data in place to be used.
Align with feedback on WordPress/wordpress-develop#6682
* add_filter( | ||
* 'script_module_data_MyScriptModuleID', | ||
* function ( array $data ): array { | ||
* $data['script-needs-this-data'] = 'ok'; | ||
* return $data; | ||
* } | ||
* ); |
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.
My only remaining question is whether adding a filter is the most ergonomic way to add data to a module. Perhaps there should be a function to make this easier? For example:
wp_script_modules()->add_data( 'MyScriptModuleID', array( 'script-needs-this-data' => 'ok' ) );
Compare with a typical way that scripts expose data to non-module scripts via wp_localize_script()
:
wp_localize_script( 'my-script-handle', 'MyScript', array( 'script-needs-this-data' => 'ok' ) );
Granted, this use of wp_localize_script()
is not ideal since it is a hack of what is intended to be used for l10n. What I think is the better more modern alternative is to use wp_add_inline_script_tag()
(which is also worse in some ways since it requires manual serialization):
wp_add_inline_script(
'my-script-handle',
sprintf( 'var MyScript = %s;', wp_json_encode( array( 'script-needs-this-data' => 'ok' ) ) ),
'before'
);
There is also the WP_Scripts::add_data()
method (and wp_script_add_data()
helper function) which are used to add "data" to script handles, where wp_localize_script()
adds this to the extra
array key, and wp_add_inline_script()
adds to the before
and after
array keys.
What if there was a similar add_data()
method on WP_Script_Modules
?
This could still ultimately get filtered, but it seems maybe a bit strange for the filter to be the primary interface to add data to a module.
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.
For example, the pending data could be stored with the registered script module:
--- a/wp-includes/class-wp-script-modules.php
+++ b/wp-includes/class-wp-script-modules.php
@@ -89,10 +89,21 @@ class WP_Script_Modules {
'version' => $version,
'enqueue' => isset( $this->enqueued_before_registered[ $id ] ),
'dependencies' => $dependencies,
+ 'data' => array(),
);
}
}
+ /**
+ * Adds data to be exported as JSON for the Script Module.
+ *
+ * @param string $id Script ID.
+ * @param array $data Data.
+ */
+ public function add_data( string $id, array $data ): void {
+ $this->registered[ $id ]['data'] = $data;
+ }
+
/**
* Marks the script module to be enqueued in the page.
*
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.
@gziolo brought this up in the Gutenberg PR: WordPress/gutenberg#61658 (comment)
The motivation for using a filter is that it's a simple interface to implement and it seems sufficient. It doesn't require the class to store any additional data or to handle data merging, the filters handle that. As you note, the filter would be compatible with other methods for adding data if there's demand for them in the future.
The patch you shared is likely sufficient, although it doesn't consider data merging. Should data be merged on subsequent ::add_data()
calls? Maybe there's an argument to merge or replace, or maybe there are separate ::set_data()
and ::add_data()
methods. As the PR stands right now, it avoids having to answer those questions.
I don't feel strongly. If you do, let me know and I can explore adding those methods.
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 don't feel strongly that they have to be added now, but as the API for the JS-side API is refined to access that data, the PHP-side API could also be refined to set the data.
Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
…62170) Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
This comment was marked as spam.
This comment was marked as spam.
Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
…62170) Update the Script Module data filter name and script tag ID to align with Core changes. See WordPress/wordpress-develop#6682. Co-authored-by: sirreal <[email protected]> Co-authored-by: gziolo <[email protected]>
Add the print_script_module_data function to the WP_Script_Modules class. Add hooks to call this function on `wp_footer` and `admin_print_footer_scripts`.
Co-authored-by: Weston Ruter <[email protected]>
0e6281c
to
c4f122e
Compare
Suggested commit message:
|
Add a new filter `script_module_data_{$module_id}` to associate data with a Script Module. For example: {{{#!php add_filter( 'script_module_data_MyScriptModuleID', function ( array $data ): array { $data['script-needs-this-data'] = 'ok'; return $data; } ); }}} If the Script Module is included in the page, enqueued or as a dependency, the associated data will be JSON-encoded and embedded in the HTML in a `<script type="application/json">` tag with an ID of the form `wp-script-module-data-{$module_id}` allowing the Script Module to access the data on the client. See the original proposal: https://make.wordpress.org/core/2024/05/06/proposal-server-to-client-data-sharing-for-script-modules/ Developed in #6682. Props jonsurrell, cbravobernal, westonruter, gziolo, bernhard-reiter, youknowriad, sergiomdgomes, czapla. Fixes #61510. See #60647. git-svn-id: https://develop.svn.wordpress.org/trunk@58579 602fd350-edb4-49c9-b593-d223f7449a82
Add a new filter `script_module_data_{$module_id}` to associate data with a Script Module. For example: {{{#!php add_filter( 'script_module_data_MyScriptModuleID', function ( array $data ): array { $data['script-needs-this-data'] = 'ok'; return $data; } ); }}} If the Script Module is included in the page, enqueued or as a dependency, the associated data will be JSON-encoded and embedded in the HTML in a `<script type="application/json">` tag with an ID of the form `wp-script-module-data-{$module_id}` allowing the Script Module to access the data on the client. See the original proposal: https://make.wordpress.org/core/2024/05/06/proposal-server-to-client-data-sharing-for-script-modules/ Developed in WordPress/wordpress-develop#6682. Props jonsurrell, cbravobernal, westonruter, gziolo, bernhard-reiter, youknowriad, sergiomdgomes, czapla. Fixes #61510. See #60647. Built from https://develop.svn.wordpress.org/trunk@58579 git-svn-id: http://core.svn.wordpress.org/trunk@58026 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Committed to Core in https://core.trac.wordpress.org/changeset/58579. |
Add a new filter `script_module_data_{$module_id}` to associate data with a Script Module. For example: {{{#!php add_filter( 'script_module_data_MyScriptModuleID', function ( array $data ): array { $data['script-needs-this-data'] = 'ok'; return $data; } ); }}} If the Script Module is included in the page, enqueued or as a dependency, the associated data will be JSON-encoded and embedded in the HTML in a `<script type="application/json">` tag with an ID of the form `wp-script-module-data-{$module_id}` allowing the Script Module to access the data on the client. See the original proposal: https://make.wordpress.org/core/2024/05/06/proposal-server-to-client-data-sharing-for-script-modules/ Developed in WordPress/wordpress-develop#6682. Props jonsurrell, cbravobernal, westonruter, gziolo, bernhard-reiter, youknowriad, sergiomdgomes, czapla. Fixes #61510. See #60647. Built from https://develop.svn.wordpress.org/trunk@58579 git-svn-id: https://core.svn.wordpress.org/trunk@58026 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Add the
print_script_module_data
function to theWP_Script_Modules
class.Register hooks to call this function on
wp_footer
andadmin_print_footer_scripts
.See the Make/Core Proposal and the PR in Gutenberg. In summary (from the proposal):
The new filter is
"script_module_data_{$module_id}"
. It should accept and return an array that will be serialized in page HTML (if it is not empty):#6683 includes an example of usage in for Core Script Modules.
Closes #6433 (superseded).
Trac ticket: https://core.trac.wordpress.org/ticket/61510
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.