Skip to content

Commit

Permalink
Fix component generator called multiple times (#6239)
Browse files Browse the repository at this point in the history
Fix registered component generator function called multiple times. Apparently this hasn't resulted in actual bugs, but it's unpredictable behaviour which can easily lead to bugs and issues.

This commit caches registered components so they are created only once.

Fixes #6232
  • Loading branch information
guyca authored May 25, 2020
1 parent 851703c commit 8ec7bcd
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 15 deletions.
4 changes: 2 additions & 2 deletions lib/src/commands/Commands.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import cloneDeep from 'lodash/cloneDeep'
import map from 'lodash/map'
import cloneDeep from 'lodash/cloneDeep';
import map from 'lodash/map';
import { CommandsObserver } from '../events/CommandsObserver';
import { NativeCommandsSender } from '../adapters/NativeCommandsSender';
import { UniqueIdProvider } from '../adapters/UniqueIdProvider';
Expand Down
24 changes: 22 additions & 2 deletions lib/src/components/ComponentRegistry.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,33 @@ import { ComponentWrapper } from './ComponentWrapper';
import { ComponentEventsObserver } from '../events/ComponentEventsObserver';
import { AppRegistryService } from '../adapters/AppRegistryService';
import { ComponentProvider } from 'react-native';
import * as React from 'react';

const DummyComponent = () => null;

class MyComponent extends React.Component<any, any> {}

describe('ComponentRegistry', () => {
let mockedStore: Store;
let mockedComponentEventsObserver: ComponentEventsObserver;
let mockedComponentWrapper: ComponentWrapper;
let mockedAppRegistryService: AppRegistryService;
let componentWrapper: ComponentWrapper;
let store: Store;
let uut: ComponentRegistry;

beforeEach(() => {
mockedStore = mock(Store);
mockedComponentEventsObserver = mock(ComponentEventsObserver);
mockedComponentWrapper = mock(ComponentWrapper);
mockedAppRegistryService = mock(AppRegistryService);
store = instance(mockedStore);
componentWrapper = instance(mockedComponentWrapper);

uut = new ComponentRegistry(
instance(mockedStore),
store,
instance(mockedComponentEventsObserver),
instance(mockedComponentWrapper),
componentWrapper,
instance(mockedAppRegistryService)
);
});
Expand All @@ -48,4 +55,17 @@ describe('ComponentRegistry', () => {
uut.registerComponent('example.MyComponent.name', generator);
expect(generator).toHaveBeenCalledTimes(0);
});

it('should wrap component only once', () => {
componentWrapper.wrap = jest.fn();
let wrappedComponent: React.ComponentClass<any>;
store.hasRegisteredWrappedComponent = jest.fn(() => wrappedComponent !== undefined);
store.setWrappedComponent = jest.fn(() => wrappedComponent = MyComponent);

const generator: ComponentProvider = jest.fn(() => DummyComponent);
uut.registerComponent('example.MyComponent.name', generator)();
uut.registerComponent('example.MyComponent.name', generator)();

expect(componentWrapper.wrap).toHaveBeenCalledTimes(1);
});
});
24 changes: 15 additions & 9 deletions lib/src/components/ComponentRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,21 @@ export class ComponentRegistry {
reduxStore?: any
): ComponentProvider {
const NavigationComponent = () => {
return this.componentWrapper.wrap(
componentName.toString(),
componentProvider,
this.store,
this.componentEventsObserver,
concreteComponentProvider,
ReduxProvider,
reduxStore
);
if (this.store.hasRegisteredWrappedComponent(componentName)) {
return this.store.getWrappedComponent(componentName);
} else {
const wrappedComponent = this.componentWrapper.wrap(
componentName.toString(),
componentProvider,
this.store,
this.componentEventsObserver,
concreteComponentProvider,
ReduxProvider,
reduxStore
);
this.store.setWrappedComponent(componentName, wrappedComponent);
return wrappedComponent;
}
};
this.store.setComponentClassForName(componentName.toString(), NavigationComponent);
this.appRegistryService.registerComponent(componentName.toString(), NavigationComponent);
Expand Down
7 changes: 7 additions & 0 deletions lib/src/components/ComponentWrapper.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,13 @@ describe('ComponentWrapper', () => {
expect(store.getComponentInstance('component1')).toBeTruthy();
});

it('Component generator is invoked only once', () => {
const componentGenerator = jest.fn(() => MyComponent);
uut.wrap(componentName, componentGenerator, store, componentEventsObserver);

expect(componentGenerator.mock.calls.length).toBe(1);
});

describe(`register with redux store`, () => {
class MyReduxComp extends React.Component<any> {
static options() {
Expand Down
4 changes: 2 additions & 2 deletions lib/src/components/ComponentWrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import { ComponentProvider } from 'react-native';
import merge from 'lodash/merge'
import merge from 'lodash/merge';
import { polyfill } from 'react-lifecycles-compat';
import hoistNonReactStatics from 'hoist-non-react-statics';

Expand Down Expand Up @@ -68,7 +68,7 @@ export class ComponentWrapper {
}

polyfill(WrappedComponent);
hoistNonReactStatics(WrappedComponent, concreteComponentProvider());
hoistNonReactStatics(WrappedComponent, concreteComponentProvider === OriginalComponentGenerator ? GeneratedComponentClass : concreteComponentProvider());
return ReduxProvider ? this.wrapWithRedux(WrappedComponent, ReduxProvider, reduxStore) : WrappedComponent;
}

Expand Down
14 changes: 14 additions & 0 deletions lib/src/components/Store.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import * as React from 'react';
import { ComponentProvider } from 'react-native';
import { IWrappedComponent } from './ComponentWrapper';

export class Store {
private componentsByName: Record<string, ComponentProvider> = {};
private propsById: Record<string, any> = {};
private componentsInstancesById: Record<string, IWrappedComponent> = {};
private wrappedComponents: Record<string, React.ComponentClass<any>> = {};

updateProps(componentId: string, props: any) {
this.propsById[componentId] = props;
Expand Down Expand Up @@ -38,4 +40,16 @@ export class Store {
getComponentInstance(id: string): IWrappedComponent {
return this.componentsInstancesById[id];
}

setWrappedComponent(componentName: string | number, wrappedComponent: React.ComponentClass<any>): void {
this.wrappedComponents[componentName] = wrappedComponent;
}

hasRegisteredWrappedComponent(componentName: string | number): boolean {
return componentName in this.wrappedComponents;
}

getWrappedComponent(componentName: string | number): React.ComponentClass<any> {
return this.wrappedComponents[componentName];
}
}

0 comments on commit 8ec7bcd

Please sign in to comment.