Skip to content
Permalink
Browse files

[sdk] Fix FacebookAds module (avoid re-rendering, update native regis…

…tration correctly)

This fixes two issues with the FacebookAds module on master:

- Avoids re-rendering when the native ad views are mounted. We need to subscribe to mounting events to get the native node handles and previously we were storing those node handles in component state, which triggers re-renders on state updates. We don't want to re-render since it sets new `ref` functions, which updates node handles in component state, which re-renders, etc. Avoiding the state updates is more efficient.
- Fix native registration updates -- the ad trigger view change check was inverted.

Test plan: open the FacebookAds example in NCL and verify we don't get warnings and that the trigger views open the advertising app in Safari and don't warn.
  • Loading branch information...
ide committed Oct 11, 2018
1 parent 88dfbf2 commit 85f2014c2aa767892a37f194ec2c86f8f36d61d4
Showing with 56 additions and 41 deletions.
  1. +1 −1 packages/expo/src/facebook-ads/AdTriggerView.tsx
  2. +55 −40 packages/expo/src/facebook-ads/withNativeAd.tsx
@@ -25,7 +25,7 @@ export default class AdTriggerView<

// Compute the context-dependent props to pass to the interactive component
let forwardedProps = this._getForwardedProps();
let props = Object.assign(forwardedProps, {
let props = Object.assign({}, forwardedProps, {
// Register the trigger component with the ad manager when it is mounted and unmounted
ref: (component: React.Component<P> | null): void => {
if (component) {
@@ -23,13 +23,16 @@ type AdContainerProps<P> = {
type AdContainerState = {
ad: NativeAd | null;
canRequestAds: boolean;
adMediaViewNodeHandle: number | null;
adIconViewNodeHandle: number | null;
interactiveTriggerNodeHandles: Map<React.Component, number>;
};

type AdProps = { nativeAd: NativeAd };

type AdNodeHandles = {
adMediaViewNodeHandle?: number | null;
adIconViewNodeHandle?: number | null;
interactiveTriggerNodeHandles?: Map<React.Component, number>;
};

/**
* A higher-order function that wraps the given `Component` type and returns a new container
* component type that passes in an extra `nativeAd` prop to the wrapped component.
@@ -43,6 +46,9 @@ export default function withNativeAd<P>(
return class NativeAdContainer extends React.Component<AdContainerProps<P>, AdContainerState> {
_subscription: EventSubscription | null = null;
_nativeAdViewRef = React.createRef<NativeAdView>();
_adMediaViewNodeHandle: number | null = null;
_adIconViewNodeHandle: number | null = null;
_interactiveTriggerNodeHandles: Map<React.Component, number> = new Map();

state: AdContainerState;

@@ -51,9 +57,6 @@ export default function withNativeAd<P>(
this.state = {
ad: null,
canRequestAds: props.adsManager.isValid,
adMediaViewNodeHandle: null,
adIconViewNodeHandle: null,
interactiveTriggerNodeHandles: new Map<React.Component, number>(),
};
}

@@ -66,25 +69,6 @@ export default function withNativeAd<P>(
}
}

componentDidUpdate(prevProps: AdContainerProps<P>, prevState: AdContainerState) {
let adMediaViewChanged = this.state.adMediaViewNodeHandle !== prevState.adMediaViewNodeHandle;
let adIconViewChanged = this.state.adIconViewNodeHandle !== prevState.adIconViewNodeHandle;
let interactiveTriggersChanged = _areEqualSets(
new Set(this.state.interactiveTriggerNodeHandles.values()),
new Set(prevState.interactiveTriggerNodeHandles.values())
);

if (adMediaViewChanged || adIconViewChanged || interactiveTriggersChanged) {
// TODO: handle unregistering views when components are unmounted
AdsManager.registerViewsForInteractionAsync(
nullthrows(findNodeHandle(this._nativeAdViewRef.current)),
this.state.adMediaViewNodeHandle !== null ? this.state.adMediaViewNodeHandle : -1,
this.state.adIconViewNodeHandle !== null ? this.state.adIconViewNodeHandle : -1,
[...this.state.interactiveTriggerNodeHandles.values()]
);
}
}

componentWillUnmount() {
if (this._subscription) {
this._subscription.remove();
@@ -132,46 +116,77 @@ export default function withNativeAd<P>(
_adMediaViewContextValue = {
nativeRef: (component: NativeAdMediaView | null) => {
if (component) {
this.setState({ adMediaViewNodeHandle: nullthrows(findNodeHandle(component)) });
this._setAdNodeHandles({ adMediaViewNodeHandle: nullthrows(findNodeHandle(component)) });
} else {
this.setState({ adMediaViewNodeHandle: null });
this._setAdNodeHandles({ adMediaViewNodeHandle: null });
}
},
};

_adIconViewContextValue = {
nativeRef: (component: NativeAdIconView | null) => {
if (component) {
this.setState({ adIconViewNodeHandle: nullthrows(findNodeHandle(component)) });
this._setAdNodeHandles({ adIconViewNodeHandle: nullthrows(findNodeHandle(component)) });
} else {
this.setState({ adIconViewNodeHandle: null });
this._setAdNodeHandles({ adIconViewNodeHandle: null });
}
},
};

_adTriggerViewContextValue = {
registerComponent: (component: React.Component) => {
let nodeHandle = nullthrows(findNodeHandle(component));
this.setState(state => {
let interactiveTriggerNodeHandles = new Map(state.interactiveTriggerNodeHandles);
interactiveTriggerNodeHandles.set(component, nodeHandle);
return { interactiveTriggerNodeHandles };
});
let interactiveTriggerNodeHandles = new Map(this._interactiveTriggerNodeHandles);
interactiveTriggerNodeHandles.set(component, nodeHandle);
this._setAdNodeHandles({ interactiveTriggerNodeHandles });
},
unregisterComponent: (component: React.Component) => {
this.setState(state => {
let interactiveTriggerNodeHandles = new Map(state.interactiveTriggerNodeHandles);
interactiveTriggerNodeHandles.delete(component);
return { interactiveTriggerNodeHandles };
});
let interactiveTriggerNodeHandles = new Map(this._interactiveTriggerNodeHandles);
interactiveTriggerNodeHandles.delete(component);
this._setAdNodeHandles({ interactiveTriggerNodeHandles });
},
onTriggerAd: () => {
if (this.state.adMediaViewNodeHandle !== null && Platform.OS === 'android') {
if (this._adMediaViewNodeHandle !== null && Platform.OS === 'android') {
let nodeHandle = findNodeHandle(this._nativeAdViewRef.current)!;
AdsManager.triggerEvent(nodeHandle);
}
},
};

/**
* Updates the registered ad views given their node handles. The node handles are not stored in
* this component's state nor does this method call "setState" to avoid unnecessarily
* re-rendering.
*/
_setAdNodeHandles({
adMediaViewNodeHandle = this._adMediaViewNodeHandle,
adIconViewNodeHandle = this._adIconViewNodeHandle,
interactiveTriggerNodeHandles = this._interactiveTriggerNodeHandles,
}: AdNodeHandles): void {
let adMediaViewChanged = adMediaViewNodeHandle !== this._adMediaViewNodeHandle;
let adIconViewChanged = adIconViewNodeHandle !== this._adIconViewNodeHandle;

let interactiveTriggersChanged = !_areEqualSets(
new Set(interactiveTriggerNodeHandles.values()),
new Set(this._interactiveTriggerNodeHandles.values())
);

if (adMediaViewChanged || adIconViewChanged || interactiveTriggersChanged) {
this._adMediaViewNodeHandle = adMediaViewNodeHandle;
this._adIconViewNodeHandle = adIconViewNodeHandle;
this._interactiveTriggerNodeHandles = interactiveTriggerNodeHandles;

// TODO: handle unregistering views when components are unmounted
if (this._adMediaViewNodeHandle !== null && this._adIconViewNodeHandle !== null) {
AdsManager.registerViewsForInteractionAsync(
nullthrows(findNodeHandle(this._nativeAdViewRef.current)),
this._adMediaViewNodeHandle,
this._adIconViewNodeHandle,
[...this._interactiveTriggerNodeHandles.values()]
);
}
}
}
};
}

0 comments on commit 85f2014

Please sign in to comment.
You can’t perform that action at this time.