-
Notifications
You must be signed in to change notification settings - Fork 826
Forms: Improve passing PHP data to Javascript #45076
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
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 3 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
'formsResponsesUrl' => $form_responses_url, | ||
'akismetActiveWithKey' => $akismet_active_with_key, | ||
'akismetUrl' => $akismet_key_url, | ||
'assetsUrl' => Jetpack_Forms::assets_url(), |
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.
In reviewing all the data we're passing to the frontend, I found these are no longer used, so I've removed them.
|
||
$data['forms'] = $forms; | ||
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.
If adopted, this new method would be the way that we pass all data in the Jetpack Forms to the frontend, replacing the 'config' on the dashboard and the jpFormsBlock object in the editor.
I've basically recreated each item from jpFormsBlock and config objects here as part of this PR. In future PRs, we can refactor individual files to use this rather than config / jpFormsBlocks. I tried doing all of that here, but it expanded to much.
Also note that I've added two new items here for canInstallPlugins and canActivatePlugins. Once merged, I'd update this PR to reference these properties rather than needing to pass both properties to both jpFormsBlocks and config separately.
self::SCRIPT_HANDLE, | ||
'window.jetpackFormsData = ' . wp_json_encode( array( 'apiRoot' => $api_root ) ) . ';', | ||
'before' | ||
); |
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.
In reviewing all the data we're passing to the frontend, I found the api_root element we set up here is not used, so I've removed it.
@@ -20,7 +21,7 @@ import './style.scss'; | |||
*/ | |||
import type { Integration } from '../../../../types'; | |||
|
|||
const isMailPoetEnabled: boolean = !! window?.jpFormsBlocks?.defaults?.isMailPoetEnabled; | |||
const isMailPoetEnabled = Boolean( getScriptData()?.forms?.isMailPoetEnabled ); |
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.
An example of replacing jpFormsBlocks method with the new getScriptsData() single-source-of-data method.
@@ -230,7 +220,6 @@ public function render_dashboard( $extra_config = array() ) { | |||
'enableIntegrationsTab' => self::$show_integrations, | |||
'renderMigrationPage' => $this->switch->is_jetpack_forms_announcing_new_menu(), | |||
'dashboardURL' => add_query_arg( 'jetpack_forms_migration_announcement_seen', 'yes', $this->switch->get_forms_admin_url() ), | |||
'isMailpoetEnabled' => Jetpack_Forms::is_mailpoet_enabled(), |
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.
As we refactor to use the getScriptsData method, we can remove each item from the config object here, as I've done with MailPoet.
@@ -246,7 +235,7 @@ public function render_dashboard( $extra_config = array() ) { | |||
* | |||
* @return boolean | |||
*/ | |||
private function has_feedback() { | |||
public function has_feedback() { |
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 had to to be made public because it's now used in the add_forms_script_data() method above.
@@ -32,7 +32,7 @@ const Integrations = () => { | |||
mailpoet: false, | |||
} ); | |||
|
|||
const isMailpoetEnabled = config( 'isMailpoetEnabled' ); | |||
const isMailpoetEnabled = Boolean( getScriptData()?.forms?.isMailPoetEnabled ); |
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.
An example of replacing the config object/method with the new getScriptData() single-source-of-data method.
forms?: FormsScriptData; | ||
} | ||
} | ||
|
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 need to extend Jetpack's script data type.
@@ -102,6 +106,9 @@ public static function init() { | |||
|
|||
// Schedule our daily cleanup | |||
add_action( 'wp_scheduled_delete', array( $instance, 'daily_akismet_meta_cleanup' ) ); | |||
|
|||
// Provide consolidated Forms script data to both editor and admin via JetpackScriptData. | |||
add_filter( 'jetpack_admin_js_script_data', array( __CLASS__, 'add_forms_script_data' ), 10, 1 ); |
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 hooks is how we call the method that adds our data to Jetpack's script data.
This has been closed in favor of #45091 |
Proposed changes:
Context I've long been bothered by the fact that we have several different methods of passing data from PHP to our frontend JavaScript.
Immediate problem. This came to a head in this PR where I wanted to pass permission checks for integrations. I had to add the permissions checks in both config and jpFormsBlocks, because the relevant component is loaded in both dashboard and editor. I had to update the component to check both config and jpFormsBlocks. And because I had to import the 'config' method from the dashboard, webpack imported the entire dashboard, which caused a build failure because of bundle size.
Solution. In this PR I propose we leverage Jetpack's built-in getScriptsData method to pass all data to the frontend. To demo it, I've done the following:
add_forms_script_data()
method that collects all the same data as all our other approaches, and attaches it to Jetpack's script data via hook. If adopted, this would be the default way we pass all data to the frontend going forward, and it will replace the config object and the jpFormsBlocks object.Follow ups. If we adopt this approach, I'd recommend we update uses of config/jpFormsBlocks to the new approach incrementally. In particular, I tried doing all of the config changes as part of this PR, and it triggered too many code changes.
Other information:
Jetpack product discussion
None.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Right now, the only place I'm using the updated method is the MailPoet check. To confirm it works in both dashboard and editor, you can...
add_filter( 'jetpack_forms_mailpoet_enable', '__return_false' );