Merge pull request #970 from gaearon/no-constructor-sideEffect

fix: construction sideEffect
gaearon · May 10, 2018 · c5c05e7 · c5c05e7
2 parents 323389a + f11b187
commit c5c05e7
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 15 deletions.
diff --git a/src/proxy/createClassProxy.js b/src/proxy/createClassProxy.js
@@ -108,18 +108,25 @@ function createClassProxy(InitialComponent, proxyKey, options) {
   let savedDescriptors = {}
   let injectedMembers = {}
   let proxyGeneration = 0
+  let classUpdatePostponed = null
+  let instancesCount = 0
   let isFunctionalComponent = !isReactClass(InitialComponent)
 
   let lastInstance = null
 
   function postConstructionAction() {
     this[GENERATION] = 0
 
+    lastInstance = this
+    // is there is an update pending
+    if (classUpdatePostponed) {
+      const callUpdate = classUpdatePostponed
+      classUpdatePostponed = null
+      callUpdate()
+    }
     // As long we can't override constructor
     // every class shall evolve from a base class
     inject(this, proxyGeneration, injectedMembers)
-
-    lastInstance = this
   }
 
   function proxiedUpdate() {
@@ -162,6 +169,7 @@ function createClassProxy(InitialComponent, proxyKey, options) {
     'componentDidMount',
     target => {
       target[PROXY_IS_MOUNTED] = true
+      instancesCount++
     },
   )
   const componentDidUpdate = lifeCycleWrapperFactory(
@@ -172,6 +180,7 @@ function createClassProxy(InitialComponent, proxyKey, options) {
     'componentWillUnmount',
     target => {
       target[PROXY_IS_MOUNTED] = false
+      instancesCount--
     },
   )
 
@@ -308,9 +317,10 @@ function createClassProxy(InitialComponent, proxyKey, options) {
       return
     }
 
+    isFunctionalComponent = !isReactClass(NextComponent)
+
     proxies.set(NextComponent, proxy)
 
-    isFunctionalComponent = !isReactClass(NextComponent)
     proxyGeneration++
 
     // Save the next constructor so we call it
@@ -343,17 +353,26 @@ function createClassProxy(InitialComponent, proxyKey, options) {
     if (isFunctionalComponent || !ProxyComponent) {
       // nothing
     } else {
-      checkLifeCycleMethods(ProxyComponent, NextComponent)
-      Object.setPrototypeOf(ProxyComponent.prototype, NextComponent.prototype)
-      defineProxyMethods(ProxyComponent, NextComponent.prototype)
-      if (proxyGeneration > 1) {
-        injectedMembers = mergeComponents(
-          ProxyComponent,
-          NextComponent,
-          InitialComponent,
-          lastInstance,
-          injectedMembers,
-        )
+      const classHotReplacement = () => {
+        checkLifeCycleMethods(ProxyComponent, NextComponent)
+        Object.setPrototypeOf(ProxyComponent.prototype, NextComponent.prototype)
+        defineProxyMethods(ProxyComponent, NextComponent.prototype)
+        if (proxyGeneration > 1) {
+          injectedMembers = mergeComponents(
+            ProxyComponent,
+            NextComponent,
+            InitialComponent,
+            lastInstance,
+            injectedMembers,
+          )
+        }
+      }
+
+      // Was constructed once
+      if (instancesCount > 0) {
+        classHotReplacement()
+      } else {
+        classUpdatePostponed = classHotReplacement
       }
     }
   }

diff --git a/test/proxy/consistency.test.js b/test/proxy/consistency.test.js
@@ -250,18 +250,21 @@ describe('consistency', () => {
             this[key] = eval(code)
           }
         }
-        /* eslint-enable */
 
         const proxy = createProxy(BaseClass)
         const Proxy = proxy.get()
         const instance = new Proxy()
         expect(instance.render()).toBe(42)
 
         proxy.update(Update1Class)
+        new Proxy() // side effect
         expect(instance.render()).toBe(43)
 
         proxy.update(Update2Class)
+        new Proxy() // side effect
+
         expect(instance.render()).toBe(42)
+        /* eslint-enable */
       })
 
       it('should reflect external dependencies', () => {
@@ -298,6 +301,7 @@ describe('consistency', () => {
             }
           }
           proxy.update(Update1Class)
+          new Proxy()
         }
         /* eslint-enable */
 
@@ -363,6 +367,80 @@ describe('consistency', () => {
       mount(<Proxy />).instance()
       expect(Proxy.isStatelessFunctionalProxy).toBe(false)
     })
+
+    it('should not update not constructed Proxies', () => {
+      const spy1 = jest.fn()
+      const spy2 = jest.fn()
+      class App extends React.Component {
+        constructor() {
+          super()
+          spy1()
+        }
+        render() {
+          return <div />
+        }
+      }
+
+      const proxy = createProxy(App)
+      expect(spy1).not.toHaveBeenCalled()
+      expect(spy1).not.toHaveBeenCalled()
+      {
+        class App extends React.Component {
+          constructor() {
+            super()
+            spy2()
+          }
+          render() {
+            return <div />
+          }
+        }
+        proxy.update(App)
+
+        expect(spy1).not.toHaveBeenCalled()
+        expect(spy1).not.toHaveBeenCalled()
+
+        const Proxy = proxy.get()
+        mount(<Proxy />)
+
+        expect(spy1).toHaveBeenCalled()
+        expect(spy1).toHaveBeenCalled()
+      }
+    })
+
+    it('should update constructed Proxies', () => {
+      const spy1 = jest.fn()
+      const spy2 = jest.fn()
+      class App extends React.Component {
+        constructor() {
+          super()
+          spy1()
+        }
+        render() {
+          return <div />
+        }
+      }
+
+      const proxy = createProxy(App)
+      const Proxy = proxy.get()
+      mount(<Proxy />)
+      expect(spy1).toHaveBeenCalled()
+      expect(spy2).not.toHaveBeenCalled()
+      {
+        class App extends React.Component {
+          constructor() {
+            super()
+            spy2()
+          }
+          render() {
+            return <div />
+          }
+        }
+        proxy.update(App)
+
+        expect(spy1).toHaveBeenCalled()
+        expect(spy2).toHaveBeenCalled()
+      }
+    })
   })
 
   describe('modern only', () => {