Tracking code return unstable references for actions

[senadir]

Quick summary

This issue is a follow up for #57449

cc @Addison-Stavlo

WPCOM tracking code will track a collection of actions, and will log something everytime that action is called.

There's a bug, in which if you have a tracked action existing inside a useSelect, the block would crash, you don't even need to call the action, this for example would break the block with a "Maximum update depth exceeded" error from the useSelect

const { createErrorNotice } = useDispatch( 'core/notices' );

useSelect( () => {
	return ( message ) => {
		createErrorNotice( message );
	};
}, [ createErrorNotice ] );

This is minimal POC with the least amount of JS code to reproduce this: https://github.com/senadir/jetpack-tracking-bug

Note: the pattern of having a function inside useSelect isn't common, but it's not unheard of, Gutenberg provides the ability to do that by having dispatch.

Steps to reproduce

For easier debugging and reproducing, this bug can be reproduced locally as well:

in wpcom, have any block or hook call this code:

const { createErrorNotice } = useDispatch( 'core/notices' );

useSelect( () => {
	return ( message ) => {
		createErrorNotice( message );
	};
}, [ createErrorNotice ] );

• Add that block to the editor.
• The block would crash.

Reproducing locally:
This repo contains a minimal POC, the wpcom minimal code is in src/tracking.js, it's an exact copy of what's in here https://github.com/Automattic/wp-calypso/blob/trunk/apps/wpcom-block-editor/src/wpcom/features/tracking.js#L767-L800 but trimmed down.

You can also reproduce this using WooCommerce Blocks 8.1.0 or less, by adding the Cart or Checkout page, some of the inner blocks would break.

What you expected to happen

The block should load fine without a bug.

What actually happened

Blocks break.

Browser

Google Chrome/Chromium

Context

No response

Platform (Simple, Atomic, or both?)

Atomic, Self-hosted

Other notes

No response

Reproducibility

Consistent

Severity

Some (< 50%)

Available workarounds?

No response

Workaround details

No response

[Addison-Stavlo]

Thanks for opening the issue and detailed description @senadir! I will get it on our board and looked into in the future.

That minimal code to repro will help, but noting:

if you have a tracked action existing inside a useSelect, the block would crash

I don't think any of the actions related to the PR that fixed the issue with the cart block issue are actually tracked on our end (?). So maybe it goes beyond actions we are actually tracking as well? 🤔

[senadir]

I will need to update that PR body, the actual problematic action was createErrorNotice which is tracked. It was not destructed inside the useSelect, but it was part of a function inside the useSelect

[Addison-Stavlo]

Gotcha, thanks for clarifying!

Initially pondering the usage this way and if we are breaking the rule of the useSelect callback needing to be a 'pure function':

I think in general we are to avoid dispatching actions inside useSelect as the function is meant to be pure and not mutate state. However, this case you propose is a bit more nuanced as we are not dispatching anything from inside the useSelect itself but just defining a function.

But maybe that is where we are breaking the rule about it being a pure function? It should have identical returns when run on the same state/inputs, but functions defined this way would not be considered identical when comparing if the component needs to update. 🤔

[senadir]

But maybe that is where we are breaking the rule about it being a pure function? It should have identical returns when run on the same state/inputs, but functions defined this way would not be considered identical when comparing if the component needs to update. 🤔

This could probably be it, however, why would this surface just with tracking :/

One thing I noticed is if I console log inside the tracking code, for example here, I get about 600 calls for page load, and 200 calls for each interaction. The code might be too heavy and trying to subscribe to too many things?

Beyond that, during my testing this seems to be the problematic code https://github.com/senadir/jetpack-tracking-bug/blob/trunk/src/tracking.js#L20-L25

Just the act of reassigning the action breaks is.

[senadir]

When I console.log originalAction, the first console.log is this:

ƒ (){return Promise.resolve(t.dispatch(e(...arguments)))}

which falls inline with the actual action, subsequent calls are all this:

ƒ () {
          if (typeof tracker === 'function') {//tracker( ...args );
          }

          return originalAction(...arguments);
        }

[Addison-Stavlo]

This could probably be it, however, why would this surface just with tracking :/

In our simple example above, perhaps that is because createErrorNotice is the dependency. If it doesn't change, that useSelect never needs to be re-evaluated.

But the first thing the tracking code does is redefine that action to be a new function, which would cause that createErrorNotice dependency in the useSelect to change and thus spark the re-evaluations. (? 🤔 )

One thing I noticed is if I console log inside the tracking code, for example here, I get about 600 calls for page load, and 200 calls for each interaction. The code might be too heavy and trying to subscribe to too many things?

We will have to look at this more closely!

[Addison-Stavlo]

and thus spark the re-evaluations. (? 🤔 )

But why more than one? .... Perhaps something to do with this:

Beyond that, during my testing this seems to be the problematic code https://github.com/senadir/jetpack-tracking-bug/blob/trunk/src/tracking.js#L20-L25

Defining the new action as an anonymous function, then if its used as a dependency its always considered to have changed when evaluated? 🤔

[senadir]

I can't yet think of a way to do this without an anonymous function.

Also removing createErrorNotice as a dependency from useSelect fixes this so you're right about that. It's just now a matter of creating a stable monkeypatched function.

[senadir]

Revisiting this again, I think the best course of action is to memoize the functions being created, this is not limited to useSelect, we saw the same problem with useLayoutEffect and useEffect. The issue is that the returned function reference is not stable.

[Addison-Stavlo]

@senadir - a bit delayed but i think #78203 should resolve what we were looking at.