Polyfills are being registered for Web Stories editor and breaking screen

westonruter commented 1 year ago

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:

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:

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 commented 1 year ago

It seems like the polyfills are being erroneously generated.

westonruter commented 1 year ago

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

thelovekesh commented 1 year ago

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 commented 1 year ago

Also to be registered on Paired Browsing.

westonruter commented 1 year ago

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

https://github.com/ampproject/amp-wp/blob/2fca9b9ca387bac50ec6f54333d5e919d098b586/src/Admin/ValidationCounts.php#L92-L93

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 commented 1 year ago

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 commented 1 year ago

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:

https://github.com/ampproject/amp-wp/blob/65f15658834d538d0dcc8cb0bf45e9668d887fdc/webpack.config.js#L208-L219

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

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 commented 1 year ago

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 commented 1 year ago

@westonruter I am not sure if Polyfills are needed with PairedBrowsing service as this service is only used when https://github.com/ampproject/amp-wp/blob/65f15658834d538d0dcc8cb0bf45e9668d887fdc/src/Admin/PairedBrowsing.php#L76

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

ampproject / amp-wp