fix: fix reconciler warnings (#852)

* Incriment generation only on replacement event(related to #843)

* hotRender: drill into the new children branch(fix #843)

* tests for the props change case + ci

* react-hot-renderer - optional child case

Author: theKashey

Diff for: examples/styled-components/src/App.js

@@ -15,7 +15,7 @@ const SmallText = emoStyled('div')`
 const indirect = {
   element: () => (
     <SmallText>
-      hidden2 <Counter />
+      hidden <Counter />
     </SmallText>
   ),
 }
@@ -24,12 +24,13 @@ const aNumber = 100500
 
 const App = () => (
   <h1>
-    <BigText>1.Hello, world!! {aNumber} </BigText>
+    <BigText>1.Hello, world! {aNumber} </BigText>
     <br />
     <SmallText>2.Hello, world.</SmallText>
     <br />
     <Counter />
     <indirect.element />
+    {aNumber % 2 && <indirect.element />}
   </h1>
 )
 

Diff for: src/proxy/utils.js

@@ -44,10 +44,11 @@ export function isNativeFunction(fn) {
 }
 
 export const identity = a => a
+const indirectEval = str => [window.eval][0](str)
 
 export const doesSupportClasses = (function() {
   try {
-    eval('class Test {}')
+    indirectEval('class Test {}')
     return true
   } catch (e) {
     return false
@@ -56,7 +57,7 @@ export const doesSupportClasses = (function() {
 
 const ES6ProxyComponentFactory =
   doesSupportClasses &&
-  eval(`
+  indirectEval(`
 (function(InitialParent, postConstructionAction) {
   return class ProxyComponent extends InitialParent {
     constructor(props, context) {

Diff for: src/reactHotLoader.js

@@ -5,6 +5,7 @@ import {
   updateProxyById,
   resetProxies,
   getProxyByType,
+  getProxyById,
   createProxyForType,
 } from './reconciler/proxies'
 
@@ -27,8 +28,14 @@ const reactHotLoader = {
       typeof fileName === 'string' &&
       fileName
     ) {
-      incrementGeneration()
-      updateProxyById(`${fileName}#${uniqueLocalName}`, type)
+      const id = `${fileName}#${uniqueLocalName}`
+
+      if (getProxyById(id)) {
+        // component got replaced. Need to reconsile
+        incrementGeneration()
+      }
+
+      updateProxyById(id, type)
     }
   },
 

Diff for: src/reconciler/hotReplacementRender.js

@@ -125,8 +125,18 @@ const mapChildren = (children, instances) => ({
         ...mapChildren(child, instances[index].children),
       }
     }
+    const instanceLine = instances[index] || {}
+    const oldChildren = asArray(instanceLine.children || [])
+    const newChildren = asArray((child.props && child.props.children) || [])
+    const nextChildren =
+      oldChildren.length && mapChildren(newChildren, oldChildren)
     return {
-      ...instances[index],
+      ...instanceLine,
+      // actually child merge is needed only for TAGs, and usually don't work for Components.
+      // the children from an instance or rendered children
+      // while children from a props is just a props.
+      // they could not exists. they could differ.
+      ...(nextChildren ? { children: nextChildren } : {}),
       type: child.type,
     }
   }),
@@ -146,7 +156,17 @@ const mergeInject = (a, b, instance) => {
   if (a.length === b.length) {
     return mapChildren(a, b)
   }
-  const flatA = unflatten(a)
+
+  // in some cases (no confidence here) B could contain A except null children
+  // in some cases - could not.
+  // this depends on React version and the way you build component.
+
+  const nonNullA = filterNullArray(a)
+  if (nonNullA.length === b.length) {
+    return mapChildren(nonNullA, b)
+  }
+
+  const flatA = unflatten(nonNullA)
   const flatB = unflatten(b)
   if (flatA.length === flatB.length) {
     return mapChildren(flatA, flatB)
@@ -239,9 +259,7 @@ const hotReplacementRender = (instance, stack) => {
       next(
         // move types from render to the instances of hydrated tree
         mergeInject(
-          filterNullArray(
-            asArray(child.props ? child.props.children : child.children),
-          ),
+          asArray(child.props ? child.props.children : child.children),
           stackChild.instance.children,
           stackChild.instance,
         ),

Diff for: src/reconciler/proxies.js

@@ -10,7 +10,8 @@ const generateTypeId = () => `auto-${elementCount++}`
 
 export const getIdByType = type => idsByType.get(type)
 
-export const getProxyByType = type => proxiesByID[getIdByType(type)]
+export const getProxyById = id => proxiesByID[id]
+export const getProxyByType = type => getProxyById(getIdByType(type))
 
 export const setStandInOptions = options => {
   renderOptions = options

Diff for: test/reactHotLoader.test.js

@@ -108,6 +108,11 @@ describe('reactHotLoader', () => {
     it('should increment update counter', () => {
       const oldGeneration = getGeneration()
       reactHotLoader.register(Div, 'Div', 'reactHotLoader.test.js')
+      // new thing, no change
+      expect(getGeneration()).toBe(oldGeneration + 0)
+
+      reactHotLoader.register(Div, 'Div', 'reactHotLoader.test.js')
+      // replacement!
       expect(getGeneration()).toBe(oldGeneration + 1)
     })
 

Diff for: test/reconciler.test.js

@@ -12,6 +12,7 @@ const spyComponent = (render, displayName, key) => {
   const mounted = jest.fn()
   const unmounted = jest.fn()
   const willUpdate = jest.fn()
+  const rendered = jest.fn()
 
   class TestingComponent extends Component {
     componentWillMount() {
@@ -34,6 +35,7 @@ const spyComponent = (render, displayName, key) => {
     /* eslint-enable */
 
     render() {
+      rendered()
       return render(this.props)
     }
   }
@@ -48,6 +50,7 @@ const spyComponent = (render, displayName, key) => {
     mounted,
     unmounted,
     key,
+    rendered,
   }
 }
 
@@ -188,6 +191,40 @@ describe('reconciler', () => {
       expect(second.mounted).toHaveBeenCalledTimes(0)
     })
 
+    it('should use new children branch during reconcile', () => {
+      const First = spyComponent(() => <u>1</u>, 'test', '1')
+      const Second = spyComponent(() => <u>2</u>, 'test', '2')
+
+      const App = ({ second }) => (
+        <div>
+          <div>
+            <First.Component />
+            {second && <Second.Component />}
+          </div>
+        </div>
+      )
+
+      const Mounter = ({ second }) => <App second={second} />
+
+      const wrapper = mount(<Mounter second />)
+
+      expect(First.rendered).toHaveBeenCalledTimes(1)
+      expect(Second.rendered).toHaveBeenCalledTimes(1)
+
+      incrementGeneration()
+      wrapper.setProps({ update: 'now' })
+      expect(First.rendered).toHaveBeenCalledTimes(3)
+      expect(Second.rendered).toHaveBeenCalledTimes(3)
+
+      incrementGeneration()
+      wrapper.setProps({ second: false })
+      expect(First.rendered).toHaveBeenCalledTimes(5)
+      expect(Second.rendered).toHaveBeenCalledTimes(3)
+
+      expect(First.unmounted).toHaveBeenCalledTimes(0)
+      expect(Second.unmounted).toHaveBeenCalledTimes(1)
+    })
+
     it('should handle function as a child', () => {
       const first = spyComponent(
         ({ children }) => <b>{children(0)}</b>,

Diff for: test/reconciler/proxyAdapter.test.js

@@ -0,0 +1,30 @@
+/* eslint-env browser */
+import { proxyWrapper } from '../../src/reconciler/proxyAdapter'
+import * as proxies from '../../src/reconciler/proxies'
+
+jest.mock('../../src/reconciler/proxies')
+
+proxies.getProxyByType.mockReturnValue({ get: () => 'proxy' })
+
+describe(`proxyAdapter`, () => {
+  const fn = () => {}
+
+  it('should handle empty result', () => {
+    expect(proxyWrapper()).toBe(undefined)
+    expect(proxyWrapper(null)).toBe(null)
+  })
+
+  it('should handle arrays', () => {
+    expect(proxyWrapper([1, 2, 3])).toEqual([1, 2, 3])
+    expect(proxyWrapper([{ type: fn, prop: 42 }])).toEqual([
+      { type: 'proxy', prop: 42 },
+    ])
+  })
+
+  it('should handle elements', () => {
+    expect(proxyWrapper({ type: fn, prop: 42 })).toEqual({
+      type: 'proxy',
+      prop: 42,
+    })
+  })
+})