Expose wordpress/element version used in the plugin

[eugene-manuilov]

Feature Description

Currently, we expose 4 libraries for external usage by 3rd party plugins: api, data, widgets, and modules. Unfortunately, these libraries aren't enough to create a 3rd party plugin that will seamlessly integrate with the SiteKit plugin.

The main problem right now is that we don't expose the version of the @wordpress/element library that we use in our plugin. It means that 3rd party developers can't use it in their plugins and have to rely on the global wp.element object that represents the version of the @wordpress/element library provided by the WordPress itself.

This is very bad because more than likely the version of the library provided by WordPress doesn't match the version that we use in our plugin which can lead to a big number of errors and crashes.

What we need to do is to expose the whole @wordpress/element library that we use in the plugin as a globally accessible object, preferably as global.googlesitekit.element.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The @wordpress/element package bundled with Site Kit should be exposed on a new element property of the googlesitekit global object
• It should be built similar to the @wordpress/i18n package
• Webpack should be bundled to source/alias @wordpress/element from the new global
• A new Asset should be registered for the new element endpoint with a handle of googlesitekit-element
• Assets that depend on it should be updated accordingly
• @wordpress/element should no longer be bundled in JS bundles other than its dedicated bundle

Implementation Brief

• Using webpack.config.js,
  • Update siteKitExternals to include @wordpress/element similar to @wordpress/i18n.
  • Update resolve to include @wordpress/element__non-shim which require @wordpress/element
  • create a new entry point for googlesitekit-element.js (see below) similar to googlesitekit-i18n
  • update React to be loaded from @wordpress/element__non-shim in ProvidePlugin
• Create assets/js/googlesitekit-element.js which exports everything from @wordpress/element__non-shim within the global googlesitekit object: global.googlesitekit.element (similar to assets/js/googlesitekit-i18n.js)
• Using includes/Core/Assets/Assets.php,
  • Add a new Script for googlesitekit-element, pointing to googlesitekit-element.js and add it as a dependency to googlesitekit-base.
• Update .eslintrc.json to include @wordpress/element__non-shim
• Update .storybook/webpack.config.js to resolve @wordpress/element similar to how it's being done for @wordpress/i18n

Test Coverage

• No new tests to be added.

QA Brief

Changelog entry

[asvinb]

Update: Getting the following error when we expose @wordpress/element:

[aaemnnosttv]

@asvinb thanks for flagging! Let's try to avoid building this out before writing the IB if possible, but it seems we may need to add some extra time to account for this issue. It's probably a matter of how webpack is configured, but we should be able to work around it.

[tofumatt]

IB ✅

[hussain-t]

QA:Eng Verified ✅

• Verified running googlesitekit.element in the browser console in (various) Site Kit screens and verified that it returns an object with exports from @wordpress/element.

[hussain-t]

@wpdarren @mohitwp I'm keeping this ticket in QA instead of moving it to approval since it needs to be verified that there are no regressions due to the changes.

[wpdarren]

QA update: ✅

Verified:

• Checked out the entire Site Kit workflow and verify that there are no regressions.
• Set up Site Kit on a new site with zero and gathering data state
• Set up Site Kit on a site with live data and made sure that the main and entity dashboard was loading the widget data.

[aaemnnosttv]

Approval ❌

While this mostly works as expected, there are a few details that need to be corrected.

• googlesitekit-element has been added as a dependency of googlesitekit-base which is the asset that is loaded across all of WP admin. As a dependency, this causes element to also be loaded on all admin screens as well.
  See Assets::enqueue_minimal_admin_script
• The script's entry point was added to our list of Module Entry Points which requires a dependency on the runtime asset (this is why googlesitekit.element does not exist on non-SK screens right now, but the script is still loaded on the page). The entry point should instead be built alongside i18n in Basic Modules which don't have the same limitations.

Initial testing with these changes shows some invalid hook call errors

The problem seems to be that we need dependencies in node_modules to also resolve react from the new external entrypoint in order to avoid creating separate instances.

If we can't solve this quickly, we may need to revert the changes for the release and try again against develop.

[nfmohit]

Thank you @aaemnnosttv!

I'm working on a follow-up based on your feedback in #6105.

[aaemnnosttv]

⚠️

After some review, I think it's best to revert the changes that were merged and try again with a fresh start here. While the current implementation seems to work as expected, it doesn't quite achieve the intended outcome here (which probably should have been more explicit) in that our wordpress/element library should essentially be enqueueable as an independent script, just like WP core. In addition to a few minor details that still need correction, the implementation is coupled to our main JS entry points, and as a result has implicit dependencies on our vendor and runtime assets.

The AC states how the exposed element asset should be built like @wordpress/i18n (i.e. as a decoupled "basic" module) which would not have the coupling as mentioned as above. This is not quite as simple as moving the entry point to the basic module group in the webpack config though as this raises problems with invalid hook call errors due to multiple react instances likely due to other dependencies from node_modules that also import react.

In the past (before we moved to webpack's shared runtime architecture) we previously solved this with a "react-shim" and additional configuration in our webpack config to ensure a singular React instance across all our assets. This further complicates things and as a non-trivial change should not consider at this point in the release cycle.

Since this issue is largely a nice-to-have infrastructural enhancement to support extensibility at some point, it's very much not critical for the release and so the merged changes should be reverted and we can land this in a future release.

[nfmohit]

As per the above comment, we have reverted the changes associated with this issue through #6111. According to further discussions in Slack, I have opened #6116 to use the learnings from this attempt and have another attempt at implementing this with the above crucial details ensured.

CC: @aaemnnosttv