Skip to content
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

Polyfills are being registered for Web Stories editor and breaking screen #7457

Closed
westonruter opened this issue Feb 7, 2023 · 14 comments · Fixed by #7458
Closed

Polyfills are being registered for Web Stories editor and breaking screen #7457

westonruter opened this issue Feb 7, 2023 · 14 comments · Fixed by #7458
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P0 High priority
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

When the latest 2.4-alpha is active along with the latest Web Stories plugin (1.29.0), accessing the "Add New" screen to create a new web story results in an error:

image

For some reason, all of AMP's polyfills are getting injected on this screen.

Strangely, when I inspect wp.domReady it is defined but it is a module and not a function:

image

Expected Behaviour

Our polyfills should only ever get injected on the AMP Settings and AMP Onboarding Wizard.

When on the plugins list screen, themes list screen, or the edit post screen: we must never enqueue our polyfills or else we risk breaking compatibility with other plugins.

Screenshots

No response

PHP Version

8.0

Plugin Version

2.4-alpha

AMP plugin template mode

Standard, Transitional, Reader

WordPress Version

6.1.1

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added Bug Something isn't working P0 High priority labels Feb 7, 2023
@westonruter westonruter added this to the v2.4 milestone Feb 7, 2023
@westonruter
Copy link
Member Author

It seems like the polyfills are being erroneously generated.

@westonruter
Copy link
Member Author

So there may be two issues: (1) polyfills being misgenerated, and (2) polyfills being registered on inappropriate screens.

@thelovekesh
Copy link
Collaborator

Strangely, when I inspect wp.domReady it is defined but it is a module and not a function:

I will investigate it further but ideally, we should only enqueue polyfills on these three screens:

  • AMP Settings page
  • AMP Onboarding Wizard
  • Support Page.

Will update the Polyfills service to take the above condition into the account. Will also take a look at how Polyfills are being bundled.

@westonruter
Copy link
Member Author

Also to be registered on Paired Browsing.

@westonruter
Copy link
Member Author

The problem seems to be with the ValidationCounts service since it is doing amp_register_polyfills on every admin screen:

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );

To resolve, I think the is_needed method on that service just needs to be updated to incorporate the same logic you did in #7421 for AfterActivationSiteScan.

@westonruter
Copy link
Member Author

With that in place, please check for other places that amp_register_polyfills may be getting triggered on screens other than AMP Settings, Onboarding Wizard, Support Page, and Paired Browsing. Any time that our code is going to be running alongside code from other plugins, and the target version of React is not active, we will need to block the functionality.

@thelovekesh
Copy link
Collaborator

thelovekesh commented Feb 7, 2023

It seems like the polyfills are being erroneously generated.

Investigated it further and it turns out it's somewhat expected as some of @wordpress/* packages use default exports for a single instance of the whole library and named exports as well. For example, there are named and default library instances in @wordpress/i18n

In the case of @wordpress/dom-ready there is only one default export of domReady function. We bundle this package as a library:

amp-wp/webpack.config.js

Lines 208 to 219 in 65f1565

output: {
devtoolNamespace: 'wp',
filename: (pathData) =>
`${
gutenbergPackages.find(
(gutenbergPackage) =>
pathData.chunk.name === gutenbergPackage.camelCaseName
).handle
}.js`,
path: path.resolve(__dirname, 'assets/js'),
library: ['wp', '[name]'],
},

when this library is bundled it creates the default instance at wp.domReady.default(in es6) because we don't have a specified library export in the Webpack config mentioned here: https://webpack.js.org/configuration/output/#outputlibraryexport

Something related @ StackOverflow: https://stackoverflow.com/questions/37912857/exporting-a-class-with-webpack-and-babel-not-working


Why our plugin is not affected by this change?

Prior to 2.4.0, the plugin has been bundled with ES5 support based on browserslist but now it get compiles the JS in ES6(seems like all target browsers support ES6 now) and ES6 evaluates if a function is an ESModule:

var getter =
	module && module.__esModule
		? function () {
				return module['default'];
			}
		: function () {
				return module;
			};

Why it breaks web-stories plugin?

web-stories also bundles with ES6 but doesn't evaluate a function in an inline script which causes the error:

<script id='web-stories-dashboard-js-after'>
	wp.domReady( function() {
	  webStories.initializeStoryDashboard( 'web-stories-dashboard', {"isRTL":false,"userId":1,"locale":{"locale":"en-US","dateFormat":"F j, Y","timeFormat":"g:i a","gmtOffset":0,"timezone":"","timezoneAbbr":"","months":["January","February","March","April","May","June","July","August","September","October","November","December"],"monthsShort":["Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"],"weekdays":["Sunday","Monday","Tuesday","Wednesday","Thursday","Friday","Saturday"],"weekdaysShort":["Sun","Mon","Tue","Wed","Thu","Fri","Sat"],"weekdaysInitials":["S","M","T","W","T","F","S"],"weekStartsOn":1},"newStoryURL":"https:\/\/loki.lndo.site\/core-dev\/build\/wp-admin\/post-new.php?post_type=web-story","archiveURL":"https:\/\/loki.lndo.site\/web-stories\/","defaultArchiveURL":"https:\/\/loki.lndo.site\/web-stories\/","cdnURL":"https:\/\/wp.stories.google\/static\/19\/","allowedImageMimeTypes":["image\/webp","image\/png","image\/jpeg","image\/gif"],"version":"1.29.0","encodeMarkup":true,"api":{"stories":"\/web-stories\/v1\/web-story\/","media":"\/web-stories\/v1\/media\/","currentUser":"\/web-stories\/v1\/users\/me\/","fonts":"\/web-stories\/v1\/fonts\/","users":"\/web-stories\/v1\/users\/","settings":"\/web-stories\/v1\/settings\/","pages":"\/wp\/v2\/pages\/","publisherLogos":"\/web-stories\/v1\/publisher-logos\/","taxonomies":"\/web-stories\/v1\/taxonomies\/","products":"\/web-stories\/v1\/products\/"},"vendors":{"none":"None","shopify":"Shopify","woocommerce":"WooCommerce"},"maxUpload":104857600,"maxUploadFormatted":"100 MB","editPostsCapabilityName":"edit_web-stories","capabilities":{"canManageSettings":true,"canUploadFiles":true},"canViewDefaultTemplates":true,"plugins":{"siteKit":{"installed":false,"active":false,"analyticsActive":false,"adsenseActive":false,"analyticsLink":"https:\/\/loki.lndo.site\/core-dev\/build\/wp-admin\/plugin-install.php?s=Site%20Kit%20by%20Google&tab=search","adsenseLink":"https:\/\/loki.lndo.site\/core-dev\/build\/wp-admin\/plugin-install.php?s=Site%20Kit%20by%20Google&tab=search"},"woocommerce":{"installed":false,"active":false,"canManage":false,"link":"https:\/\/loki.lndo.site\/core-dev\/build\/wp-admin\/plugin-install.php?s=WooCommerce&tab=search"}},"flags":{"enableSVG":false,"enableInProgressTemplateActions":false},"globalAutoAdvance":true,"globalPageDuration":7} );
	} );
</script>

@thelovekesh
Copy link
Collaborator

Any time that our code is going to be running alongside code from other plugins, and the target version of React is not active, we will need to block the functionality.

Apart from Onboarding Wizard, Options Page, and Support Page, only the themes/plugins admin pages were using React which is already blocked for React >=18.

@thelovekesh
Copy link
Collaborator

@westonruter I am not sure if Polyfills are needed with PairedBrowsing service as this service is only used when

Services::get( 'dependency_support' )->has_support()

and this service JS has dependecy on wp-dom-ready and wp-url which are already present in current WP versions and have stable APIs.

@westonruter
Copy link
Member Author

In regards to the wp.domReady error, I'm still not clear on how our plugin is not affected. Take for example, the following plugin which adds a script to every admin screen:

<?php
/**
 * Plugin Name: domReady Console Log
 */

add_action('admin_enqueue_scripts', function () {
    wp_enqueue_script('wp-dom-ready');
    wp_add_inline_script( 'wp-dom-ready', 'wp.domReady(() => {console.log("DOM READY!")});', 'after' );
});

Currently, this works properly on every screen except where the polyfills are registered, in which case it outputs:

Uncaught TypeError: wp.domReady is not a function

When polyfills are registered, wp.domReady is unexpectedly a module object as opposed to a function. This would breaks compatibility with other plugins that are expecting normal functions to be exported.

@thelovekesh
Copy link
Collaborator

@westonruter Because in current JS bundling it is being converted to something like this:

<?php
/**
 * Plugin Name: domReady Console Log
 */

add_action('admin_enqueue_scripts', function () {
    wp_enqueue_script('wp-dom-ready');
    wp_add_inline_script( 'wp-dom-ready', '
	wp.domReady = wp.domReady && wp.domReady.__esModule ? wp.domReady.default : wp.domReady;
	wp.domReady(() => {console.log("DOM READY!")});
	', 'after' );
});

@westonruter
Copy link
Member Author

OK, well, we'll need to change the bundling to output the same as whatever WordPress core outputs for wp-dom-ready.js and other scripts.

@thelovekesh
Copy link
Collaborator

QA Passed ✅

  • Tested with plugins like Web Stories, Jetpack, Gutenberg, and a few others plugins on the dev playground, and no site functionality was found broken and no console errors were generated by the AMP plugin.
  • Polyfills are only being forced on only AMP-powered screens and no other admin screens.
  • PairedBrowsing is working as expected and no polyfills are being enqueued by the AMP plugin in PairedBrowsing.
  • ValidationCounts is being enqueued to all admin screens and working as expected and not enforcing the AMP plugin to add any polyfills.
  • Customizer is working as expected and not enforcing the AMP plugin to add any polyfills.
  • Packaged by AMP plugin for polyfilling and backward compatibility, the wp-dom-ready package is working as expected and returns domReady function instance now instead of the ES Module when checked in window.wp global via browser console.
  • SiteScanNotice only works when React 17 is there else it doesn't function at all in themes/plugins admin screen.
  • There are no ReactDOM render deprecation warnings generated by the AMP plugin.

cc/ @westonruter @pavanpatil1

@pavanpatil1
Copy link

Tested it with a few plugins (web stories, Woocommerece, Gutenberg, Yoast) It is working fine no console error/logs related to the plugin are displayed.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P0 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants