[RNMobile] Prevent calling the registerComponent callback multiple times #32985
Conversation
fluiddot
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.07 MB ℹ️ View Unchanged
|
👋 @fluiddot ! Gave a first look at the solution here and even though I understand how it works, I don't feel particularly confident about fighting against the current lifecycle of the app and components by keeping a static variable. I feel that this is a indication that the lifecycle is not clean enough (on one hand) and that adding a static variable to combat it will only move the problem elsewhere. I understand that this feeling is not super specific or concrete though. Since the issue here only happens in the development build, I wonder if we should just spend some more time finding a different solution, one that would probably leave the component lifecycle in a better state? Let me know what you think. |
My initial approach when I tackled this issue was to figure out why we have some differences between the production and development modes, like the reusable blocks being unsupported. After the investigation, I realized that was related to the App component's lifecycle and logging errors/warnings, so I wanted to propose this PR as a workaround but I agree that is more like a hack than an actual solution. I have some ideas that I'd like to explore so I'll set the PR to draft and propose a new solution. Thanks for reviewing it 🙇 ! |
| return cloneElement( element, filteredProps ); | ||
| } ); | ||
| componentDidMount() { | ||
| doAction( 'native.render', this.filteredProps ); |
There was a problem hiding this comment.
I understand that the action native.render should happen after the component is rendered, so now this will be executed after the component is mounted, this means after the first render.
| 'native.block_editor_props', | ||
| parentProps | ||
| ); | ||
| doAction( 'native.pre-render', parentProps ); |
There was a problem hiding this comment.
The native.pre-render action will be executed right before the component is mounted, this means before the first render.
There was a problem hiding this comment.
I do not follow exactly how the "ceremony" of a class addresses the original issue of multiple calls to native.pre-render. Is render/AppRegistry.registerComponent still called multiple times? Is the class instance retained between the calls?
There was a problem hiding this comment.
As far as I checked, the multiple calls are caused when returning a function instead of a component when registering the component in AppRegistry. For example, if you replace line 41 with the following code:
AppRegistry.registerComponent( id, () => ( props ) => {
debugger;
return <App { ...props } />;
} );
The debugger will pause for every warning and error that happens during the initialization, I haven't located yet the exact part from the React Native code that causes this. My impression is that when a warning/error is handled and tries to get the component stack, for some reason is calling the function returned from the callback passed in the AppRegistry.registerComponent call.
I haven't checked with a functional component, but in this case, since we need to execute the pre-render action on the constructor, I haven't found a reliable way to do it with a functional component so I used a classic component.
There was a problem hiding this comment.
The debugger will pause for every warning and error that happens during the initialization, I haven't located yet the exact part from the React Native code that causes this. My impression is that when a warning/error is handled and tries to get the component stack, for some reason is calling the function returned from the callback passed in the
AppRegistry.registerComponentcall.
Ah, I see. From your statement and debugging myself a little further my perception is that the component registered with AppRegistry is merely re-rendering when these warnings/errors occur. I presume that is expected, but I have no explicit source to back up that presumption. However, I now understand the doAction calls to be side effects and shouldn't be invoked on every render.
I haven't checked with a functional component, but in this case, since we need to execute the pre-render action on the constructor, I haven't found a reliable way to do it with a functional component so I used a classic component.
From a brief test, wrapping the side effects with useEffect does appear to have the same outcome, i.e. preventing multiple runs, but there is not explicit reason to avoid a class. So, leveraging the class approach is OK with me.
Functional App
/**
* External dependencies
*/
import { AppRegistry } from 'react-native';
import { omit } from 'lodash';
/**
* WordPress dependencies
*/
import { applyFilters, doAction } from '@wordpress/hooks';
import { useEffect } from '@wordpress/element';
/**
* Internal dependencies
*/
import { cloneElement } from './react';
const render = ( element, id ) =>
AppRegistry.registerComponent( id, () => ( propsFromParent ) => {
const parentProps = omit( propsFromParent || {}, [ 'rootTag' ] );
useEffect( () => {
doAction( 'native.pre-render', parentProps );
}, [] );
const filteredProps = applyFilters(
'native.block_editor_props',
parentProps
);
useEffect( () => {
doAction( 'native.render', filteredProps );
}, [] );
return cloneElement( element, filteredProps );
} );
/**
* Render a given element on Native.
* This actually returns a componentProvider that can be registered with `AppRegistry.registerComponent`
*
* @param {WPElement} element Element to render.
*/
export { render };In regards to "we need to execute the pre-render action on the constructor," is this to imply that doAction( 'native.render', filteredProps ); (or other code) is dependent upon doAction( 'native.pre-render', parentProps );? Or was your statement merely your interpretation based upon the existing name "pre-render"?
There was a problem hiding this comment.
The debugger will pause for every warning and error that happens during the initialization, I haven't located yet the exact part from the React Native code that causes this. My impression is that when a warning/error is handled and tries to get the component stack, for some reason is calling the function returned from the callback passed in the
AppRegistry.registerComponentcall.Ah, I see. From your statement and debugging myself a little further my perception is that the component registered with
AppRegistryis merely re-rendering when these warnings/errors occur. I presume that is expected, but I have no explicit source to back up that presumption. However, I now understand thedoActioncalls to be side effects and shouldn't be invoked on every render.
Exactly, the doAction calls should be treated as side effects and only executed once.
Regarding why the function is being called, it's a bit hard to follow the trace but it's related to how the react-devtools package builds the component stacktrace when a warning/error is triggered, here are some screenshots to clarify it:
1. Log warning is triggered
2. Getting the component stack (react-dev-tools)
The important part here is the current variable that holds the current Fiber node and is passed down the stack trace.
Here you can see that the type property of the current Fiber node is the App function:
3. Describe function component frame
The previous current value is passed as the workInProgress argument in the function shown in the screenshot, note that the first argument of the call to describeFunctionComponentFrame is the type value (the App function) that was mentioned previosly.
4. Describe native component frame - App function call
I haven't checked with a functional component, but in this case, since we need to execute the pre-render action on the constructor, I haven't found a reliable way to do it with a functional component so I used a classic component.
From a brief test, wrapping the side effects with
useEffectdoes appear to have the same outcome, i.e. preventing multiple runs, but there is not explicit reason to avoid a class. So, leveraging the class approach is OK with me.
Thanks for sharing the functional component approach 🙇 ! I agree that wrapping the doAction calls with useEffect is enough to prevent calling them multiple times, but I decided to go with the class approach because I thought that the native.pre-render call should happen before the component is rendered and useEffect is triggered after the first render. Due to this reason is why I added the native.pre-render action in the class contructor, I found some workarounds to run code on a functional component before the first render but looked like hacks.
In regards to "we need to execute the pre-render action on the constructor," is this to imply that
doAction( 'native.render', filteredProps );(or other code) is dependent upondoAction( 'native.pre-render', parentProps );? Or was your statement merely your interpretation based upon the existing name "pre-render"?
As far as I checked in the file where the actions are defined, I don't feel there are any dependency between the actions but I infer from the action's name native.pre-render that this should be called before the editor is rendered 🤔 .
There was a problem hiding this comment.
Regarding why the function is being called, it's a bit hard to follow the trace but it's related to how the
react-devtoolspackage builds the component stacktrace when a warning/error is triggered [...]
Yeah, I did notice — if I remembering correctly — the original multiple side effect calls issue only occurs if Debug is enabled from the developer menu.
As far as I checked in the file where the actions are defined, I don't feel there are any dependency between the actions but I infer from the action's name
native.pre-renderthat this should be called before the editor is rendered 🤔 .
Makes sense. I have no data to suggest otherwise at this point. The class constructor approach does ensure it invokes before initial render.
There was a problem hiding this comment.
Yeah, I did notice — if I remembering correctly — the original multiple side effect calls issue only occurs if Debug is enabled from the developer menu.
I think it also happens when the Debug is disabled because the reusable blocks are still not showing up in that case. I infer it's being called multiple times because the reusable block is being unregistered when this code gets executed a second time, surprisingly the capabilities object is empty and since the reusable block has been already registered, the condition is true.
gutenberg/packages/react-native-editor/src/index.js
Lines 60 to 66 in db76f67
Makes sense. I have no data to suggest otherwise at this point. The class constructor approach does ensure it invokes before initial render.
👍
| AppRegistry.registerComponent( id, () => ( propsFromParent ) => { | ||
| const parentProps = omit( propsFromParent || {}, [ 'rootTag' ] ); | ||
| const render = ( element, id ) => { | ||
| class App extends Component { |
There was a problem hiding this comment.
The App class component is defined within the render function because it uses the element argument to render the component.
fluiddot
force-pushed
the
rnmobile/fix/prevent-pre-render-hook-multiple-times
branch
from
dc27a49 to
f392e04
Compare
| 'native.block_editor_props', | ||
| parentProps | ||
| ); | ||
| doAction( 'native.pre-render', parentProps ); |
There was a problem hiding this comment.
I do not follow exactly how the "ceremony" of a class addresses the original issue of multiple calls to native.pre-render. Is render/AppRegistry.registerComponent still called multiple times? Is the class instance retained between the calls?
There was a problem hiding this comment.
LGTM. 👍🏻 My earlier #32985 (review) verified the app functioned as expected. Thank you for sorting this out. 🙇🏻
| 'native.block_editor_props', | ||
| parentProps | ||
| ); | ||
| doAction( 'native.pre-render', parentProps ); |
There was a problem hiding this comment.
Regarding why the function is being called, it's a bit hard to follow the trace but it's related to how the
react-devtoolspackage builds the component stacktrace when a warning/error is triggered [...]
Yeah, I did notice — if I remembering correctly — the original multiple side effect calls issue only occurs if Debug is enabled from the developer menu.
As far as I checked in the file where the actions are defined, I don't feel there are any dependency between the actions but I infer from the action's name
native.pre-renderthat this should be called before the editor is rendered 🤔 .
Makes sense. I have no data to suggest otherwise at this point. The class constructor approach does ensure it invokes before initial render.
Description
On development mode, I noticed that the callback executed when registering a component in the App registry is called multiple times. This case happens when the code within the callback triggers any warning or error (this means calling
console.warningorconsole.error), in this issue there's additional helpful information regarding this.This issue has been addressed by adding a static object that contains the components that have been already registered, this way we enforce that the callback's code will be executed only once.EDIT: I finally followed a different approach to address this problem, based on the
AppRegistrydocumentation, I created a class component that holds the initialization logic.How has this been tested?
debugger;before this lineVerify that reusable blocks are rendered
The original issue was spotted due to the reusable blocks being rendered as unsupported blocks so it would be nice to verify that the issue is no longer happening.
Screenshots
N/A
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).