Serializer: Fix Invariant error related to hooks

[nerrad]

Description

This is an error that currently doesn't seem to be surfacing anywhere in the interface for users but was spotted by @aduth when testing #13879 against the work merged in from #15737 (introducing useSelect and implemented in `withSelect).

For his tests he was doing something like this in the console:

ExampleComponent = wp.data.withSelect( () => ( {} ) )( () => wp.element.createElement( 'div', null, 'Howdy There' ) );
wp.element.renderToString( wp.element.createElement( ExampleComponent ) );

This produced the error:

Uncaught Invariant Violation: Hooks can only be called inside the body of a function component.

This was troubleshot for a few minutes in slack and taking cues from some of the things uncovered there I spent some time trying out a few things and ended up with this rough pull which allows for renderToString to run without errors for the above code snippet (producing the expected <div>Howdy There</div> output). However, I don't think this is the solution we'll roll with (won't we need hooks to actually run?). I decided to throw this pull up anyways and it can be iterated on with ideas for solving.

Note:

• I did try feeding the actual hooks to the monkey patched Dispatcher but that resulted in recursion errors (try it and you'll see). Actual error is Uncaught RangeError: Maximum call stack size exceeded
• I'm not very familiar with React internals so this is a bit of a shot in the dark at resolving this issue.
• I'm kinda surprised this isn't surfacing in any interface or e2e testing problems (yet)... kinda makes me wonder if there's something different about reproducing this in the console with the test string.

[gziolo]

😅

[nerrad]

moderator note there was some spam commenting on this issue. I reported the user to github and blocked them from this repo

[nerrad]

This was mostly just to get the ball rolling on one option which is monkey patching the ReactCurrentDispatcher before doing the render. However, if we go this route, we'll probably have to reproduce similar logic for hooks found here. This seems fragile and increases maintenance burden. The other (maybe not significant) potential issue is that essentially any logic in a useEffect or useLayoutEffect hook in a rendered component will not fire if we use something similar to ReactPartialRendererHooks because this serializer is effectively doing similar to what server type tasks do (as opposed to browser).

If renderToString is mostly used for the save function of blocks (stateless components) I wonder if we should simply detect usages of withSelect/hooks and simply NOT support them (throw errors?).

[nerrad]

This is a great article to read that provides insight to the api around the renderers: https://overreacted.io/how-does-setstate-know-what-to-do/

Essentially GB's custom renderer will have to implement it's own dispatcher to support hooks (assuming it's desired that hooks are supported)

[aduth]

I'm kinda surprised this isn't surfacing in any interface or e2e testing problems (yet)... kinda makes me wonder if there's something different about reproducing this in the console with the test string.

There are two things which prevent it from being more obviously-problematic:

• It's an issue of element renderToString, which for the large part is isolated to a block's save component. Since hooks are used in implementing statefulness of components, and since a save implementation should be pure, there's rarely overlap.
• Even if it did occur, the end-to-end tests don't yet run with SCRIPT_DEBUG and thus wouldn't log the warning to be captured by the global console logging trap. See Testing: Use SCRIPT_DEBUG in end-to-end test environment #14845.

[pascalknecht]

I think I've just encountered this issue too. I want to have access to the current post in the save function as I want to use the current url for a share functionality. Here is the code which produces the error:

registerBlockType('wk/share', {
	title: 'Share',
	attributes: {},
	save: ShareSave
});

const ShareSave = (props) => {
	const post = useSelect( (select) => {
		return select('core/editor').getCurrentPost();
	});
       // Need access to the link here
       return (<div></div>);
};

Any ideas @aduth @nerrad ? Can this be solved differently?

[WunderBart]

I've just bumped into the same issue. In my case, the reason was Webpack bundling second, different version of React (16.4) from the one globally available through Gutenberg (16.8), just like here: facebook/react#15270. Externalizing React and ReactDOM did the trick for me.

[nerrad]

@pascalknecht from the docs around the save function:

Note: The save function should be a pure function that depends only on the attributes used to invoke it.

In other words, it shouldn't depend on external sources outside of the attributes for your block. This is one of the reasons why this isn't a high priority issue right now.

What you might want to consider doing is make whatever value you are constructing as a useSelect in your current ShareSave component a block attribute and expose it that way to the component via something you do in the registered block edit function.

[nerrad]

@WunderBart Thanks for reporting your findings in here but on the surface it does not sound like you were experiencing the same issue. The error being reported in this issue has nothing to do with specific React version - so I'm guessing you encountered another invariant error (potentially related to hooks) and thought these were the same issues. Glad you found a solution for your issue though 😄

[kauhat]

Hi folks, I've been running into this one a lot over the past week as I didn't want to bake some theme level settings into an attribute for every block.

As a workaround, I'm using a shim of the useSelect function to catch the error and fall back to the global object.

import { useSelect as baseUseSelect } from '@wordpress/data';

export function useSelect( mapper ) {
	try {
		// Use builtin selector.
		return baseUseSelect( mapper );
	} catch ( e ) {
		// Re-throw if exception type is not expected.
		if ( 'Invariant Violation' !== e.name ) {
			throw e;
		}

		// Pass global data object to mapper. (No props + side effects?)
		return mapper( wp.data.select );
	}
}

This is ugly and I have no idea what sort of side effects this has - but it seems to be working in my current project where that part of the state isn't changed between editor load and save.

Note that the props argument will be empty, so I'm setting the mapper function props with a default value. For example:

// ...

save( { attributes } ) {
	// Using shim function. (2nd arg defaults to this scope's save() props.)
	const { availableBreakpoints } = useSelect( ( select, props = arguments[0] ) => {
		return {
			availableBreakpoints: select( 'example/grid' ).getBreakpoints()
		}
	} );

	console.log(availableBreakpoints)

	// etc...
},

I have a feeling this is a terrible idea, so please don't rely on this!

[aduth]

@nerrad Per @kauhat's comment, do you think it might be realistic to consider this as an allowable error using a simple try / catch?

I haven't dug into it too much, but I see that errors in React are coded, so it might be something we could reliably identify:

https://github.com/facebook/react/blob/b64938e1234b77c50d9b2680dd836cd27ce5ad91/scripts/error-codes/codes.json#L299

I guess, per your original comment, it depends if we need the hooks to run. Did your original approach here solve that problem?

[nerrad]

I guess, per your original comment, it depends if we need the hooks to run. Did your original approach here solve that problem?

The original approach basically used a custom dispatcher that replaced existing hooks with noop. So effectively it prevents the errors. A try/catch could do something similar I suppose. I'd be wary about referencing the original hook as that risks introducing side-effects into save which we want to avoid right?

[aduth]

Oh, I was thinking that the try / catch could exist in the serializer implementation, not referencing or modifying any specific hook.

[nerrad]

Oh, I was thinking that the try / catch could exist in the serializer implementation, not referencing or modifying any specific hook.

I was thinking the same too, what were you thinking the catch would do, just silently discard the error and allow things to continue?

[aduth]

Oh, I was thinking that the try / catch could exist in the serializer implementation, not referencing or modifying any specific hook.

I was thinking the same too, what were you thinking the catch would do, just silently discard the error and allow things to continue?

Pretty much this, yes. As long as we're able to reliably limit it to this very specific error, and throw all other errors as normal.

[nerrad]

Now that withInstanceId is using a hook under the hood, there is more potential for this kind of error surfacing. If I get some time I'll try out the latest suggestion by @aduth here but I just wanted to make this note.

[annezazu]

Closing this out due to inactivity and in order to keep current PRs more relevant. Welcome folks to open a new PR to address this issue as it arises going forward and am happy to reopen this if that's preferred!