Decouple module-specific effects from core components

[aaemnnosttv]

Feature Description

As a general principal, module-specific code can depend on core infrastructure, but not the other way around. Core infra being core data stores as one of the most explicit examples, but also things like very top-level components like DashboardMainApp.

The SK dashboards have a well-defined API for adding elements to the dashboard but the concept of widgets is directly associated with layout. Sometimes we want to invoke various module-specific side-effects which in the past have either been shoe-horned into an existing component, or added into a higher level component where they don't really belong.

There have been a few instances recently where we've wanted to use such functionality where it hasn't been available such as synchronizing module settings and creating custom dimensions to name a few.

One solution to this is to use the Widgets API – even today this can be used to accomplish the end goal.

Example

widgets.registerWidget(
    'analyticsDashboardEffects',
    {
        Component( { WidgetNull } ) {
            useEffect( () => {
                console.log( 'analyticsDashboardEffects:effect-run' );

                return () => console.log( 'analyticsDashboardEffects:effect-cleanup' );
            }, [] );

            return <WidgetNull />;
        },
        width: widgets.WIDGET_WIDTHS.FULL, // Required but irrelevant.
        priority: 0,
        modules: [ 'analytics-4' ],
    },
    [
        AREA_MAIN_DASHBOARD_TRAFFIC_PRIMARY,
    ]
);

This registers a widget on the main dashboard for the sole purpose of running effects. This will work, and by using WidgetNull unconditionally, the layout will never be affected by the "empty" widget. This could be combined with the whenActive() HOC for guarding it to only run when a module is active or connected. It isn't entirely ideal as-is here as there are things like width that aren't relevant, nor should we need to render WidgetNull to avoid throwing a wrench in the layout when it isn't concerned with layout at all.

One idea is to register a new widget area that would always be hidden which could be used for this kind of thing, and maybe we build a bit of a layer on top to avoid needing to define properties that aren't relevant for this kind of widget/area. Most of the parts of the Widget API are still relevant though like declaring modules for use with Dashboard Sharing, so it doesn't seem that a new API of sorts would be warranted.

---

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

Acceptance criteria

• All use of module-specific actions, selectors, and components should be removed from Dashboard*App and DashboardEntryPoint components and moved to module-registered locations
  • These should only run when their respective modules are connected in most cases (if not, please note these exceptions) and guarded accordingly to prevent invoking

Note: the example in the description is not a requirement but could be used as an part of an initial implementation. It could be worth exploring what a proper API for this would look like, although there would likely be substantial overlap with features already provided by the widgets API.

Implementation Brief

• [ ] In assets/js/googlesitekit/modules/index.js
  • Update the documentation header for the registerModule to list two new optional components: MainRootComponent and EntityRootComponent
• [ ] Update assets/js/googlesitekit/modules/datastore/modules.js
  • Include two more props in the registerModule action - MainRootComponent and EntityRootComponent and add them to the settings variable
• [ ] Add assets/js/components/ModuleRootComponents.js
  • Include one prop - dashboardType, it should be string enum, holding either main or entity value
  • Use getModules selector from CORE_MODULES store to fetch all modules
  • Iterate over returned modules and filter for the ones that have property connected and depending on the dashboardType prop, if it is main then filter in for the modules that have MainRootComponent property, otherwise filter for EntityRootComponent and save the results in a variable, say filteredModules for example
  • If filteredModules is empty return early
  • Otherwise, iterate over the filteredModules and render their MainRootComponent or EntityRootComponent depending on the value of dashboardType prop
• [ ] Update assets/js/components/DashboardEntryPoint.js
  • Render ModuleRootComponents together with either ModuleSetup or DashboardMainApp depending on the current value of setupModuleSlug
• [ ] Update assets/js/components/DashboardEntityApp.js
  • Render ModuleRootComponents component after ScrollEffect component. Pass the dashboardType prop with value of entity.
• [ ] Extract:
  • Custom dimensions logic from DashboardMainApp and syncGoogleTagSettings call with it's useMount hook from DashboardEntryPoint into a separate component assets/js/modules/analytics-4/components/MainRootComponent.js, which only contain hooks, and return null in the renderer.
• [ ] Pass MainRootComponent component in assets/js/modules/analytics-4/index.js to the registerModule function as MainRootComponent property.

Test Coverage

• Add tests to the assets/js/googlesitekit/modules/datastore/modules.test.js to verify new components. You can use accepts settings components for the module and defaults settings components tonullif not provided as starting point

QA Brief

• Do overall smoke test, also around creating custom dimensions and Google tag settings syncing which now only happen on the main dashboard
• Verify that everything works as expected

Changelog entry

• Update module infrastructure to add proper support for dashboard effects.

[jamesozzie]

One more report of this error in the support forums. Awaiting user feedback.

[adamdunnage]

One more report of this to add here.

User has shared their SH Info and confirmed that they don't encounter the same from an Incognito window. I will continue to troubleshoot this with the user and add any more updates here if I get them.

[eugene-manuilov]

Thanks, @zutigrm, but please do not use the PR as IB approach because it is strongly discouraged. If you can't write an IB without coding it beforehand, then just leave it for someone else. By spending time on your PoC, you spend time on implementation in the current sprint for a thing that is supposed to be implemented in a following sprint, thus our planning for that sprint will less accurate. Plus it is not always the case that PoC will be needed, thus time spent on it will be wasted.

Add assets/js/googlesitekit/widgets/datastore/effects.js

• Add registerModuleEffect action, which should be a minimal adaptation from registerWidget action, only handling slug and effect - callback hook that will be passed

This is a nice start, but I don't think it should be done this way. The problem here is that you try to squeeze module related functionality into the widgets domain that should know nothing about that. We need to separate concerns.

The more appropriate solution would be updating the registerModule function to accept two new properties, let's say MainRootComponent and EntityRootComponent (similarly to SettingsEditComponent, SettingsViewComponent, etc). These two components will be responsible for all side effects that a module needs to do and will be rendered on MainDashboard and EntityDashboard respectively.

Using components approach will let us perform side effects that rely on certain criteria to happen and can be deferred in time.

• syncGoogleTagSettings from DashboardEntryPoint into a separate hook

Looks like there is no syncGoogleTagSettings in the DashboardEntryPoint component anymore.

[zutigrm]

@eugene-manuilov Thanks for the feedback/clarification and new route suggestion. I updated IB to reflect that approach.

[eugene-manuilov]

Thanks, @zutigrm. Mostly looks good.

Add assets/js/googlesitekit/modules/ModuleEffects.js

Let's put it into the assets/js/components folder and call it ModuleRootComponents.

Update assets/js/components/DashboardMainApp.js

We need to update the DashboardEntryPoint component to render ModuleRootComponents. It should render the root components element + either ModuleSetup or DashboardMainApp depending on the setupModuleSlug value.

Extract:

• Custom dimensions logic from DashboardMainApp into a separate component assets/js/modules/analytics-4/dashboard/SideEffects.js, which only contain hooks, and return null in the renderer

• Let's put it into the assets/js/modules/analytics-4/components folder and call it MainRootComponent.
• We also need to move syncGoogleTagSettings from DashboardEntryPoint into this new component: https://github.com/google/site-kit-wp/blob/f885564ff92344bdb6fd0f4fd2a9c64492e3335b/assets/js/components/DashboardEntryPoint.js#L36-L42

[zutigrm]

@eugene-manuilov Thanks, IB updated

[eugene-manuilov]

Thanks, @zutigrm. IB ✔️

[aaemnnosttv]

This is good to go now but we should only merge it after the split for the release. Assigning to @tofumatt to do that afterwards 👍

[jamesozzie]

Just a note to mention there are a couple more reports of this users impacted by this over the past few days. Details below:

1. https://wordpress.org/support/topic/sitekit-encountered-an-error-not-working/
2. https://wordpress.org/support/topic/site-kit-encountered-an-error-131/