Fix button with react component doesn't get unmounted (wix#5457)

yogevbd · web-flow · commit a8175ef272ef · 2019-09-05T16:17:11.000+03:00
* Fix button with react component doesn't get unmounted

* Test e2e only on iOS
diff --git a/e2e/Options.test.js b/e2e/Options.test.js
@@ -88,6 +88,12 @@ describe('Options', () => {
     await expect(elementByLabel('Styling Options')).toBeVisible();
   });
 
+  it(':ios: Reseting buttons should unmount button react view', async () => {
+    await elementById(TestIDs.SHOW_LIFECYCLE_BTN).tap();
+    await elementById(TestIDs.RESET_BUTTONS).tap();
+    await expect(elementByLabel('Button component unmounted')).toBeVisible();
+  });
+
   xit('hides topBar onScroll down and shows it on scroll up', async () => {
     await elementById(TestIDs.PUSH_OPTIONS_BUTTON).tap();
     await elementById(TestIDs.SCROLLVIEW_SCREEN_BUTTON).tap();
diff --git a/lib/ios/RNNNavigationButtons.m b/lib/ios/RNNNavigationButtons.m
@@ -103,7 +103,7 @@ -(RNNUIBarButtonItem*)buildButton: (NSDictionary*)dictionary defaultStyle:(RNNBu
 		componentOptions.name = [[Text alloc] initWithValue:component[@"name"]];
 		
 		RNNReactView *view = [_componentRegistry createComponentIfNotExists:componentOptions parentComponentId:self.viewController.layoutInfo.componentId reactViewReadyBlock:nil];
-		barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withCustomView:view];
+		barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withCustomView:view componentRegistry:_componentRegistry];
 	} else if (iconImage) {
 		barButtonItem = [[RNNUIBarButtonItem alloc] init:buttonId withIcon:iconImage];
 	} else if (title) {
diff --git a/lib/ios/RNNReactComponentRegistry.h b/lib/ios/RNNReactComponentRegistry.h
@@ -11,6 +11,8 @@
 
 - (void)removeComponent:(NSString *)componentId;
 
+- (void)removeChildComponent:(NSString *)childId;
+
 - (void)clearComponentsForParentId:(NSString *)parentComponentId;
 
 - (void)clear;
diff --git a/lib/ios/RNNReactComponentRegistry.m b/lib/ios/RNNReactComponentRegistry.m
@@ -48,6 +48,16 @@ - (void)removeComponent:(NSString *)componentId {
 	}
 }
 
+- (void)removeChildComponent:(NSString *)childId {
+	NSMutableDictionary* parent;
+	while ((parent = _componentStore.objectEnumerator.nextObject)) {
+		if ([parent objectForKey:childId]) {
+			[parent removeObjectForKey:childId];
+			return;
+		}
+	}
+}
+
 - (void)clear {
 	[_componentStore removeAllObjects];
 }
diff --git a/lib/ios/RNNReactView.h b/lib/ios/RNNReactView.h
@@ -12,4 +12,6 @@ typedef void (^RNNReactViewReadyCompletionBlock)(void);
 
 - (void)setAlignment:(NSString *)alignment inFrame:(CGRect)frame;
 
+- (NSString *)componentId;
+
 @end
diff --git a/lib/ios/RNNReactView.m b/lib/ios/RNNReactView.m
@@ -22,13 +22,17 @@ - (void)contentDidAppear:(NSNotification *)notification {
 	
 	RNNReactView* appearedView = notification.object;
 	
-	 if (_reactViewReadyBlock && [appearedView.appProperties[@"componentId"] isEqual:self.appProperties[@"componentId"]]) {
+	 if (_reactViewReadyBlock && [appearedView.appProperties[@"componentId"] isEqual:self.componentId]) {
 	 	_reactViewReadyBlock();
 		 _reactViewReadyBlock = nil;
 		 [[NSNotificationCenter defaultCenter] removeObserver:self];
 	 }
 }
 
+- (NSString *)componentId {
+	return self.appProperties[@"componentId"];
+}
+
 - (void)setRootViewDidChangeIntrinsicSize:(void (^)(CGSize))rootViewDidChangeIntrinsicSize {
 		_rootViewDidChangeIntrinsicSize = rootViewDidChangeIntrinsicSize;
 		self.delegate = self;
diff --git a/lib/ios/RNNUIBarButtonItem.h b/lib/ios/RNNUIBarButtonItem.h
@@ -1,14 +1,15 @@
 #import <Foundation/Foundation.h>
 #import <React/RCTRootView.h>
 #import <React/RCTRootViewDelegate.h>
+#import "RNNReactComponentRegistry.h"
 
 @interface RNNUIBarButtonItem : UIBarButtonItem <RCTRootViewDelegate>
 
 @property (nonatomic, strong) NSString* buttonId;
 
 -(instancetype)init:(NSString*)buttonId withIcon:(UIImage*)iconImage;
 -(instancetype)init:(NSString*)buttonId withTitle:(NSString*)title;
--(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView*)reactView;
+-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView componentRegistry:(RNNReactComponentRegistry *)componentRegistry;
 -(instancetype)init:(NSString*)buttonId withSystemItem:(NSString*)systemItemName;
 
 @end
diff --git a/lib/ios/RNNUIBarButtonItem.m b/lib/ios/RNNUIBarButtonItem.m
@@ -7,6 +7,7 @@ @interface RNNUIBarButtonItem ()
 
 @property (nonatomic, strong) NSLayoutConstraint *widthConstraint;
 @property (nonatomic, strong) NSLayoutConstraint *heightConstraint;
+@property (nonatomic, weak) RNNReactComponentRegistry *componentRegistry;
 
 @end
 
@@ -28,9 +29,10 @@ -(instancetype)init:(NSString*)buttonId withTitle:(NSString*)title {
 	return self;
 }
 
--(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView {
+-(instancetype)init:(NSString*)buttonId withCustomView:(RCTRootView *)reactView componentRegistry:(RNNReactComponentRegistry *)componentRegistry {
 	self = [super initWithCustomView:reactView];
 	
+	self.componentRegistry = componentRegistry;
 	reactView.sizeFlexibility = RCTRootViewSizeFlexibilityWidthAndHeight;
 	reactView.delegate = self;
 	reactView.backgroundColor = [UIColor clearColor];
@@ -76,4 +78,11 @@ - (void)onButtonPressed {
 					  afterDelay:0];
 }
 
+- (void)dealloc {
+	if ([self.customView isKindOfClass:[RNNReactView class]]) {
+		RNNReactView* customView = self.customView;
+		[self.componentRegistry removeChildComponent:customView.componentId];
+	}
+}
+
 @end
diff --git a/playground/src/screens/LifecycleButton.js b/playground/src/screens/LifecycleButton.js
@@ -0,0 +1,9 @@
+const RoundedButton = require('./RoundedButton');
+
+class LifecycleButton extends RoundedButton {
+  componentWillUnmount() {
+    alert('Button component unmounted');
+  }
+}
+
+module.exports = LifecycleButton;
diff --git a/playground/src/screens/OptionsScreen.js b/playground/src/screens/OptionsScreen.js
@@ -1,5 +1,5 @@
 const React = require('react');
-const { Component } = require('react');
+const {Component} = require('react');
 const Root = require('../components/Root');
 const Button = require('../components/Button')
 const Navigation = require('../services/Navigation');
@@ -16,7 +16,9 @@ const {
   PUSH_BTN,
   HIDE_TOPBAR_DEFAULT_OPTIONS,
   SHOW_YELLOW_BOX_BTN,
-  SET_REACT_TITLE_VIEW
+  SET_REACT_TITLE_VIEW,
+  RESET_BUTTONS,
+  SHOW_LIFECYCLE_BTN
 } = require('../testIDs');
 
 class Options extends Component {
@@ -69,10 +71,36 @@ class Options extends Component {
         <Button label='Set React Title View' testID={SET_REACT_TITLE_VIEW} onPress={this.setReactTitleView} />
         <Button label='Show Yellow Box' testID={SHOW_YELLOW_BOX_BTN} onPress={() => console.warn('Yellow Box')} />
         <Button label='StatusBar' onPress={this.statusBarScreen} />
+        <Button label='Show Lifecycle button' testID={SHOW_LIFECYCLE_BTN} onPress={this.showLifecycleButton} />
+        <Button label='Remove all buttons' testID={RESET_BUTTONS} onPress={this.resetButtons} />
       </Root>
     );
   }
 
+  showLifecycleButton = () => Navigation.mergeOptions(this, {
+    topBar: {
+      rightButtons: [
+        {
+          id: 'ROUND',
+          testID: ROUND_BUTTON,
+          component: {
+            name: Screens.LifecycleButton,
+            passProps: {
+              title: 'Two'
+            }
+          }
+        }
+      ]
+    }
+  });
+
+  resetButtons = () => Navigation.mergeOptions(this, {
+    topBar: {
+      rightButtons: [],
+      leftButtons: []
+    }
+  });
+
   changeTitle = () => Navigation.mergeOptions(this, {
     topBar: {
       title: {
diff --git a/playground/src/screens/Screens.js b/playground/src/screens/Screens.js
@@ -26,6 +26,7 @@ module.exports = {
   Navigation: 'Navigation',
   NativeScreen: 'RNNCustomComponent',
   RoundButton: 'CustomRoundedButton',
+  LifecycleButton: 'LifecycleButton',
   ReactTitleView: 'ReactTitleView',
   EventsScreen: 'EventsScreen',
   EventsOverlay: 'EventsOverlay',
diff --git a/playground/src/screens/index.js b/playground/src/screens/index.js
@@ -28,6 +28,9 @@ function registerScreens() {
   Navigation.registerComponent(Screens.Overlay, () => require('./OverlayScreen'));
   Navigation.registerComponent(Screens.OverlayAlert, () => require('./OverlayAlert'));
   Navigation.registerComponent(Screens.Pushed, () => require('./PushedScreen'));
+  Navigation.registerComponent(Screens.ScrollViewOverlay, () => require('./ScrollViewOverlay'));
+  Navigation.registerComponent(Screens.RoundButton, () => require('./RoundedButton'));
+  Navigation.registerComponent(Screens.LifecycleButton, () => require('./LifecycleButton'));
   Navigation.registerComponent(Screens.ReactTitleView, () => require('./CustomTopBar'));
   Navigation.registerComponent(Screens.RoundButton, () => require('./RoundedButton'));
   Navigation.registerComponent(Screens.ScrollViewOverlay, () => require('./ScrollViewOverlay'));
diff --git a/playground/src/testIDs.js b/playground/src/testIDs.js
@@ -128,6 +128,8 @@ module.exports = {
   PUSH_BOTTOM_TABS_BUTTON: `PUSH_BOTTOM_TABS_BUTTON`,
   SET_STACK_ROOT_BUTTON: `SET_STACK_ROOT_BUTTON`,
   SET_ROOT:'SET_ROOT',
+  RESET_BUTTONS: 'RESET_BUTTONS',
+  SHOW_LIFECYCLE_BTN: 'SHOW_LIFECYCLE_BTN',
 
   // Elements
   SCROLLVIEW_ELEMENT: `SCROLLVIEW_ELEMENT`,

Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,16 @@ - (void)removeComponent:(NSString *)componentId {`
`48`	`48`	`}`
`49`	`49`	`}`
`50`	`50`
	`51`	`+- (void)removeChildComponent:(NSString *)childId {`
	`52`	`+ NSMutableDictionary* parent;`
	`53`	`+ while ((parent = _componentStore.objectEnumerator.nextObject)) {`
	`54`	`+ if ([parent objectForKey:childId]) {`
	`55`	`+ [parent removeObjectForKey:childId];`
	`56`	`+ return;`
	`57`	`+ }`
	`58`	`+ }`
	`59`	`+}`
	`60`	`+`
`51`	`61`	`- (void)clear {`
`52`	`62`	`[_componentStore removeAllObjects];`
`53`	`63`	`}`