Packages: Add package babel-plugin-transform-with-select

[aduth]

Alternative to: #12877

This pull request seeks to explore using a Babel transform to provide a known subset of reducer keys by which to limit subscriber callbacks for connected components.

See: Proof of Concept

Implementation notes:

Effectively, this adds support for both subscribe and withSelect to accept an optional second argument, a string or array of strings corresponding to the concerned reducer keys, where the callback is called if and only if the changed store is included in the subset of reducer keys.

This is then leveraged by a custom Babel transform plugin which automatically injects this argument based on the calls to select within the mapSelectToProps function of a withSelect-connected component. Unlike #12877, these keys are determined at build-time, rather than at runtime, and is thus not prone to the same false negatives described there.

While initial proposals for such an argument starting at #12877 (comment) considered it as the first argument, the changes here propose it as a second argument. Since it's an optional argument, it is easier both to implement and to document to consider as a second argument. At some point, I could imagine this argument could be overloaded as an object of options, on which reducerKeys (namespaces?) would be one of a set of possible properties.

Remaining tasks:

Initial performance measurements have shown to be disappointingly non-impacting. I plan to spend more time here, as there should be a noted impact, particularly on initial page load for posts containing non-trivial amounts of content.

After which point, remaining items include:

• Consider whether to support aliasing of withSelect and select, rather than the current implementation which depends on hard-coded identifiers
• Consider a canonical name for "reducer keys", as our current use of reducerKey doesn't seem strictly accurate to describe this namespace concept, particularly in mind of Redux agnosticism as of wp.data: Split store implementation out from registry. #10289.
• Automated tests
• Inline code commenting
• Package documentation, missing boilerplate

cc @jsnajdr

[jsnajdr]

I like this optimization 🙂 Because it's an optional optimization, it should be 100% correct and should bail out in situation where correctness is not guaranteed:

// we don't know what `passAsParam` selects from, optimizing just for `core/foo` is wrong
withSelect( select => {
  return {
    a: passAsParam( select ),
    b: select( 'core/foo' ).getFoo(),
  };
} )

// the current transform doesn't catch call to `s`, optimizing just for `core/foo` is wrong
withSelect( select => {
  const s = select;
  return {
    a: select( 'core/foo' ).getFoo(),
    b: s( 'core/bar' ).getBar(),
  };
} )

// the current transform doesn't optimize this, but at least does no harm
withSelect( s => {
  return {
    a: s( 'core/foo' ).getFoo(),
  };
} )

Even with this transform, I still like the idea of "single-store select" and would love to see it implemented:

withSelect( 'core/foo', store => {
  return { a: store.getFoo() };
} );

I like how it's "optimized by default" and doesn't rely on a Babel transform that is somewhat magical.

Consider a canonical name for "reducer keys",

💯 I'm repeatedly confused by the reducerKey name. I think storeName is a better fit these days. The reducer is an implementation detail that the user of the store doesn't see.

[aduth]

Even with this transform, I still like the idea of "single-store select" and would love to see it implemented:

Maybe then, in order to keep this option available, while I'd not implement it here, it might be good for me to at least avoid overloading the "hint" argument to be a singular string, and instead expect it to always be an array if the function is to receive select as the argument and not the object of the store's selectors.

[jsnajdr]

Maybe then, in order to keep this option available, while I'd not implement it here, it might be good for me to at least avoid overloading the "hint" argument to be a singular string, and instead expect it to always be an array

Limiting the hint argument to be an array certainly doesn't hurt. Especially given that it will be generated by a Babel transform and rarely provided by the programmer in the source. The usual benefit of such overloading, better DX and code readability, is not there.

But even if the hint could be also a string, there is no conflict with the "single-store-select" API, is there? The two forms are:

withSelect( mapSelect: function, hint?: Array<string> | string )
withSelect( storeName: string, mapSelectors: function )

The implementation can always figure out what form is used by a few simple typeofs.

[aduth]

No, I suppose not, with that specific order argument at least. It just wasn't clear to me that the difference in usage would be obvious where, as you note, in one case it's a mapSelect and the other a mapSelectors. In any case, it's something which can be worked on separate from the changes here.

[aduth]

There are good ideas here, but with consideration of registry-enhanced selectors as introduced by #13662, the ability to determine store dependencies from a withSelect has become much more complex than this naive implementation can support.

I'd still like to explore options forward here, and suspect it would inherit major parts of what was implemented here, but for now this can't serve as a working solution in its current form.

[aduth]

Brain-dumping so it doesn't get lost: With the constraints applied to createRegistrySelector via #13889, it now behaves very similarly to withSelect, and much like we did here, I could imagine a build transform which either assigns a property to the selector function or provides an argument to the createRegistrySelector call with the store names referenced within the bound selector. At that point, if we surface each selector referenced in a mapSelectToProps, we could recursively derive dependent stores.

Source:

withSelect( ( select ) => {
	return select( 'core' ).isRequestingEmbedPreview( '...' );
} )( MyComponent );

// ...

const isRequestingEmbedPreview = createRegistrySelector( ( select ) => ( state, url ) => {
	return select( 'core/data' ).isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
} );

Transformed:

withSelect( ( select ) => {
	return select( 'core' ).isRequestingEmbedPreview( '...' );
}, [ [ 'core', [ 'isRequestingEmbedPreview' ] ] ] )( MyComponent );

// ...

const isRequestingEmbedPreview = createRegistrySelector( ( select ) => ( state, url ) => {
	return select( 'core/data' ).isResolving( REDUCER_KEY, 'getEmbedPreview', [ url ] );
} );

isRequestingEmbedPreview.dependencies = [ [ 'core/data', [ 'isResolving' ] ] ];

Then to get all dependencies:

const getStoreDependencies = ( dependencies ) => uniq( flatMap(
	dependencies,
	( [ storeName, selectorNames ] ) => [
		storeName,
		...selectorNames.map( ( selectorName ) => (
			getStoreDependencies( select( storeName )[ selectorName ].dependencies )
		) )
	]
) );

withSelect = ( mapSelectToProps, dependencies ) => {
	const storeDependencies = getStoreDependencies( dependencies );
	// [ 'core', 'core/data' ]
};

Certainly not anywhere near as clean a solution as what was previously possible, but there might be a viable implementation somewhere in here all the same.

[aduth]

• Consider a canonical name for "reducer keys", as our current use of reducerKey doesn't seem strictly accurate to describe this namespace concept, particularly in mind of Redux agnosticism as of wp.data: Split store implementation out from registry. #10289.

To ensure this is not lost, I created a tracking issue at #14150.