Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer reading native view config from UIManager until view is used #10569

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/renderers/native/NativeMethodsMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@ import type {
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
} from 'ReactNativeTypes';
import type {
ReactNativeBaseComponentViewConfig,
} from 'ReactNativeViewConfigRegistry';
} from 'ReactNativeTypes';

/**
* `NativeMethodsMixin` provides methods to access the underlying native
Expand Down
4 changes: 1 addition & 3 deletions src/renderers/native/ReactNativeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,8 @@ import type {
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
} from 'ReactNativeTypes';
import type {
ReactNativeBaseComponentViewConfig,
} from 'ReactNativeViewConfigRegistry';
} from 'ReactNativeTypes';

/**
* Superclass that provides methods to access the underlying native component.
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/native/ReactNativeFiberEntry.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const ReactNativeFiber: ReactNativeType = {
ReactNativeComponentTree: require('ReactNativeComponentTree'), // InspectorUtils, ScrollResponder
ReactNativePropRegistry: require('ReactNativePropRegistry'), // flattenStyle, Stylesheet
TouchHistoryMath: require('TouchHistoryMath'), // PanResponder
createReactNativeComponentClass: require('createReactNativeComponentClass'), // eg Text
createReactNativeComponentClass: require('createReactNativeComponentClass'), // eg RCTText, RCTView, ReactNativeART
takeSnapshot: require('takeSnapshot'), // react-native-implementation
},
};
Expand Down
4 changes: 1 addition & 3 deletions src/renderers/native/ReactNativeFiberHostComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ import type {
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
ReactNativeBaseComponentViewConfig,
} from 'ReactNativeTypes';
import type {Instance} from 'ReactNativeFiberRenderer';
import type {
ReactNativeBaseComponentViewConfig,
} from 'ReactNativeViewConfigRegistry';

/**
* This component defines the same methods as NativeMethodsMixin but without the
Expand Down
4 changes: 1 addition & 3 deletions src/renderers/native/ReactNativeFiberRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ const deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationI
const emptyObject = require('fbjs/lib/emptyObject');
const invariant = require('fbjs/lib/invariant');

import type {
ReactNativeBaseComponentViewConfig,
} from 'ReactNativeViewConfigRegistry';
import type {ReactNativeBaseComponentViewConfig} from 'ReactNativeTypes';

export type Container = number;
export type Instance = {
Expand Down
17 changes: 10 additions & 7 deletions src/renderers/native/ReactNativeTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ export type MeasureLayoutOnSuccessCallback = (
height: number,
) => void;

export type ReactNativeBaseComponentViewConfig = {
validAttributes: Object,
uiViewClassName: string,
propTypes?: Object,
};

export type ViewConfigGetter = () => ReactNativeBaseComponentViewConfig;

/**
* This type keeps ReactNativeFiberHostComponent and NativeMethodsMixin in sync.
* It can also provide types for ReactNative applications that use NMM or refs.
Expand All @@ -51,16 +59,11 @@ export type NativeMethodsMixinType = {
setNativeProps(nativeProps: Object): void,
};

type ReactNativeBaseComponentViewConfig = {
validAttributes: Object,
uiViewClassName: string,
propTypes?: Object,
};

type SecretInternalsType = {
NativeMethodsMixin: NativeMethodsMixinType,
createReactNativeComponentClass(
viewConfig: ReactNativeBaseComponentViewConfig,
name: string,
callback: ViewConfigGetter,
): any,
ReactNativeComponentTree: any,
ReactNativePropRegistry: any,
Expand Down
50 changes: 37 additions & 13 deletions src/renderers/native/ReactNativeViewConfigRegistry.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,29 +14,53 @@

const invariant = require('fbjs/lib/invariant');

export type ReactNativeBaseComponentViewConfig = {
validAttributes: Object,
uiViewClassName: string,
propTypes?: Object,
};
import type {
ReactNativeBaseComponentViewConfig,
ViewConfigGetter,
} from 'ReactNativeTypes';

const viewConfigCallbacks = new Map();
const viewConfigs = new Map();

const ReactNativeViewConfigRegistry = {
register(viewConfig: ReactNativeBaseComponentViewConfig) {
const name = viewConfig.uiViewClassName;
/**
* Registers a native view/component by name.
* A callback is provided to load the view config from UIManager.
* The callback is deferred until the view is actually rendered.
* This is done to avoid causing Prepack deopts.
*/
register(name: string, callback: ViewConfigGetter): string {
invariant(
!viewConfigs.has(name),
!viewConfigCallbacks.has(name),
'Tried to register two views with the same name %s',
name,
);
viewConfigs.set(name, viewConfig);
viewConfigCallbacks.set(name, callback);
return name;
},
get(name: string) {
const config = viewConfigs.get(name);
invariant(config, 'View config not found for name %s', name);
return config;

/**
* Retrieves a config for the specified view.
* If this is the first time the view has been used,
* This configuration will be lazy-loaded from UIManager.
*/
get(name: string): ReactNativeBaseComponentViewConfig {
let viewConfig;
if (!viewConfigs.has(name)) {
const callback = viewConfigCallbacks.get(name);
invariant(
typeof callback === 'function',
'View config not found for name %s',
name,
);
viewConfigCallbacks.set(name, null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this isn't .delete? Either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use viewConfigCallbacks to check for view name uniqueness. Nulling out prevents us from blocking GC while avoiding having to check both viewConfigs and viewConfigCallbacks maps.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

viewConfig = callback();
viewConfigs.set(name, viewConfig);
} else {
viewConfig = viewConfigs.get(name);
}
invariant(viewConfig, 'View config not found for name %s', name);
return viewConfig;
},
};

Expand Down
16 changes: 8 additions & 8 deletions src/renderers/native/__tests__/ReactNativeEvents-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ beforeEach(() => {
it('handles events', () => {
expect(RCTEventEmitter.register.mock.calls.length).toBe(1);
var EventEmitter = RCTEventEmitter.register.mock.calls[0][0];
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {foo: true},
uiViewClassName: 'View',
});
}));

var log = [];
ReactNative.render(
Expand Down Expand Up @@ -94,10 +94,10 @@ it('handles events on text nodes', () => {
expect(RCTEventEmitter.register.mock.calls.length).toBe(1);
var EventEmitter = RCTEventEmitter.register.mock.calls[0][0];

var Text = createReactNativeComponentClass({
var Text = createReactNativeComponentClass('Text', () => ({
validAttributes: {foo: true},
uiViewClassName: 'Text',
});
}));

class ContextHack extends React.Component {
static childContextTypes = {isInAParentText: PropTypes.bool};
Expand Down Expand Up @@ -179,10 +179,10 @@ it('handles events on text nodes', () => {

it('handles when a responder is unmounted while a touch sequence is in progress', () => {
var EventEmitter = RCTEventEmitter.register.mock.calls[0][0];
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {id: true},
uiViewClassName: 'View',
});
}));

function getViewById(id) {
return UIManager.createView.mock.calls.find(
Expand Down Expand Up @@ -273,10 +273,10 @@ it('handles when a responder is unmounted while a touch sequence is in progress'

it('handles events without target', () => {
var EventEmitter = RCTEventEmitter.register.mock.calls[0][0];
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {id: true},
uiViewClassName: 'View',
});
}));

function getViewById(id) {
return UIManager.createView.mock.calls.find(
Expand Down
24 changes: 12 additions & 12 deletions src/renderers/native/__tests__/ReactNativeMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ describe('ReactNative', () => {
});

it('should be able to create and render a native component', () => {
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {foo: true},
uiViewClassName: 'View',
});
}));

ReactNative.render(<View foo="test" />, 1);
expect(UIManager.createView).toBeCalled();
Expand All @@ -40,10 +40,10 @@ describe('ReactNative', () => {
});

it('should be able to create and update a native component', () => {
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {foo: true},
uiViewClassName: 'View',
});
}));

ReactNative.render(<View foo="foo" />, 11);

Expand All @@ -61,10 +61,10 @@ describe('ReactNative', () => {
});

it('should not call UIManager.updateView after render for properties that have not changed', () => {
const Text = createReactNativeComponentClass({
const Text = createReactNativeComponentClass('Text', () => ({
validAttributes: {foo: true},
uiViewClassName: 'Text',
});
}));

ReactNative.render(<Text foo="a">1</Text>, 11);
expect(UIManager.updateView).not.toBeCalled();
Expand All @@ -87,10 +87,10 @@ describe('ReactNative', () => {
});

it('should not call UIManager.updateView from setNativeProps for properties that have not changed', () => {
const View = createReactNativeComponentClass({
const View = createReactNativeComponentClass('View', () => ({
validAttributes: {foo: true},
uiViewClassName: 'View',
});
}));

class Subclass extends ReactNative.NativeComponent {
render() {
Expand Down Expand Up @@ -122,10 +122,10 @@ describe('ReactNative', () => {
});

it('returns the correct instance and calls it in the callback', () => {
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {foo: true},
uiViewClassName: 'View',
});
}));

var a;
var b;
Expand All @@ -143,10 +143,10 @@ describe('ReactNative', () => {
});

it('renders and reorders children', () => {
var View = createReactNativeComponentClass({
var View = createReactNativeComponentClass('View', () => ({
validAttributes: {title: true},
uiViewClassName: 'View',
});
}));

class Component extends React.Component {
render() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,14 @@ describe('createReactNativeComponentClass', () => {
uiViewClassName: 'View',
};

const Text = createReactNativeComponentClass(textViewConfig);
const View = createReactNativeComponentClass(viewViewConfig);
const Text = createReactNativeComponentClass(
textViewConfig.uiViewClassName,
() => textViewConfig,
);
const View = createReactNativeComponentClass(
viewViewConfig.uiViewClassName,
() => viewViewConfig,
);

expect(Text).not.toBe(View);

Expand All @@ -53,10 +59,16 @@ describe('createReactNativeComponentClass', () => {
uiViewClassName: 'Text', // Same
};

createReactNativeComponentClass(textViewConfig);
createReactNativeComponentClass(
textViewConfig.uiViewClassName,
() => textViewConfig,
);

expect(() => {
createReactNativeComponentClass(altTextViewConfig);
createReactNativeComponentClass(
altTextViewConfig.uiViewClassName,
() => altTextViewConfig,
);
}).toThrow('Tried to register two views with the same name Text');
});
});
16 changes: 8 additions & 8 deletions src/renderers/native/createReactNativeComponentClass.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,21 @@

const ReactNativeViewConfigRegistry = require('ReactNativeViewConfigRegistry');

// See also ReactNativeBaseComponent
export type ReactNativeBaseComponentViewConfig = {
validAttributes: Object,
uiViewClassName: string,
propTypes?: Object,
};
import type {ViewConfigGetter} from 'ReactNativeTypes';

/**
* Creates a renderable ReactNative host component.
* Use this method for view configs that are loaded from UIManager.
* Use createReactNativeComponentClass() for view configs defined within JavaScript.
*
* @param {string} config iOS View configuration.
* @private
*/
const createReactNativeComponentClass = function(
viewConfig: ReactNativeBaseComponentViewConfig,
name: string,
callback: ViewConfigGetter,
): string {
return ReactNativeViewConfigRegistry.register(viewConfig);
return ReactNativeViewConfigRegistry.register(name, callback);
};

module.exports = createReactNativeComponentClass;