From 1f5dc022ab88adba4aa3e832a4b07dec208538e8 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Sat, 14 Sep 2019 09:35:05 +0200 Subject: [PATCH 1/7] (chore) - start refactor to Provider check instead of Consumer --- src/create-context.js | 23 +++++----- test/browser/createContext.test.js | 70 +++++++++++++++++++++++++----- 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/src/create-context.js b/src/create-context.js index 36a17e936e..a4ffd5c6f8 100644 --- a/src/create-context.js +++ b/src/create-context.js @@ -13,11 +13,6 @@ export function createContext(defaultValue) { _id: '__cC' + i++, _defaultValue: defaultValue, Consumer(props, context) { - - this.shouldComponentUpdate = function (_props, _state, _context) { - return _context !== context || props.children !== _props.children; - }; - return props.children(context); }, Provider(props) { @@ -27,14 +22,16 @@ export function createContext(defaultValue) { ctx[context._id] = this; return ctx; }; - this.shouldComponentUpdate = props => { - subs.some(c => { - // Check if still mounted - if (c._parentDom) { - c.context = props.value; - enqueueRender(c); - } - }); + this.shouldComponentUpdate = _props => { + if (props.value !== _props.value) { + subs.some(c => { + // Check if still mounted + if (c._parentDom) { + c.context = props.value; + enqueueRender(c); + } + }); + } }; this.sub = (c) => { subs.push(c); diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index f44747230d..b218861ced 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -355,30 +355,41 @@ describe('createContext', () => { const { Provider, Consumer } = createContext(); const CONTEXT = { i: 1 }; + class NoUpdate extends Component { + shouldComponentUpdate() { + return false; + } + + render() { + return this.props.children; + } + } + class Inner extends Component { render(props) { + console.log('rendering', props.i); return
{props.i}
; } } sinon.spy(Inner.prototype, 'render'); - const Child = data => ; - render( - - {Child} - + + + {data => } + + , scratch ); render( - - {Child} - + + {data => } + , scratch ); @@ -389,18 +400,55 @@ describe('createContext', () => { render( - - {Child} - + + {data => } + , scratch ); // Rendered three times, should call 'Consumer' render two times + // Logged these things I think we're dealing with another stale dom issue due to NoUpdate... :/ expect(Inner.prototype.render).to.have.been.calledTwice.and.calledWithMatch({ i: 2 }, {}, { ['__cC' + (ctxId - 1)]: {} }); expect(scratch.innerHTML).to.equal('
2
'); }); + it('should allow for updates of props', () => { + let set; + const MyContext = createContext(); + class App extends Component { + constructor(props) { + super(props); + this.state = { + x: 'y' + }; + set = this.setState.bind(this); + this.renderInner = this.renderInner.bind(this); + } + + renderInner() { + return

{this.state.x}

; + } + + render() { + return ( + + + {this.renderInner} + + + ); + } + } + + render(, scratch); + expect(scratch.innerHTML).to.equal('

y

'); + + set({ x: 'x' }); + rerender(); + expect(scratch.innerHTML).to.equal('

x

'); + }); + it('should re-render the consumer if the children change', () => { const { Provider, Consumer } = createContext(); const CONTEXT = { i: 1 }; From 4d339fb803bea09e9f198abf38ca1bf8ea4b7771 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Tue, 17 Sep 2019 11:50:17 +0300 Subject: [PATCH 2/7] Set the provider value to the new one since in diff its old --- src/create-context.js | 3 ++- test/browser/createContext.test.js | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/create-context.js b/src/create-context.js index a4ffd5c6f8..7daae35348 100644 --- a/src/create-context.js +++ b/src/create-context.js @@ -24,10 +24,11 @@ export function createContext(defaultValue) { }; this.shouldComponentUpdate = _props => { if (props.value !== _props.value) { + ctx[context._id].props.value = _props.value; subs.some(c => { // Check if still mounted if (c._parentDom) { - c.context = props.value; + c.context = _props.value; enqueueRender(c); } }); diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index b218861ced..0bdeff15cb 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -3,6 +3,7 @@ import { createElement as h, render, Component, createContext } from '../../src/ import { setupScratch, teardown } from '../_util/helpers'; import { Fragment } from '../../src'; import { i as ctxId } from '../../src/create-context'; +import options from '../../src/options'; /** @jsx h */ @@ -352,6 +353,10 @@ describe('createContext', () => { }); it('should not re-render the consumer if the context doesn\'t change', () => { + + // Disable the debounce rendering + options.debounceRendering = f => f(); + const { Provider, Consumer } = createContext(); const CONTEXT = { i: 1 }; @@ -408,7 +413,6 @@ describe('createContext', () => { ); // Rendered three times, should call 'Consumer' render two times - // Logged these things I think we're dealing with another stale dom issue due to NoUpdate... :/ expect(Inner.prototype.render).to.have.been.calledTwice.and.calledWithMatch({ i: 2 }, {}, { ['__cC' + (ctxId - 1)]: {} }); expect(scratch.innerHTML).to.equal('
2
'); }); From 0a4b20892207b543f333c9b8a19cc35738b5e5b2 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Tue, 17 Sep 2019 11:51:53 +0300 Subject: [PATCH 3/7] Remove the console from context test --- test/browser/createContext.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index 0bdeff15cb..b23f9e99af 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -372,7 +372,6 @@ describe('createContext', () => { class Inner extends Component { render(props) { - console.log('rendering', props.i); return
{props.i}
; } } From b02ac83d400faa360453c3ee360dc302dba45c17 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Tue, 17 Sep 2019 15:22:26 +0300 Subject: [PATCH 4/7] Added back the sCU optimzations for context.Consumer --- src/create-context.js | 3 ++ test/browser/createContext.test.js | 53 ++++++++++++++++-------------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/create-context.js b/src/create-context.js index 7daae35348..8c56c47b65 100644 --- a/src/create-context.js +++ b/src/create-context.js @@ -13,6 +13,9 @@ export function createContext(defaultValue) { _id: '__cC' + i++, _defaultValue: defaultValue, Consumer(props, context) { + this.shouldComponentUpdate = function (_props, _state, _context) { + return context !== _context || _props.children === props.children; + } return props.children(context); }, Provider(props) { diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index b23f9e99af..854a65dacf 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -1,9 +1,8 @@ -import { setupRerender } from 'preact/test-utils'; +import { setupRerender, act } from 'preact/test-utils'; import { createElement as h, render, Component, createContext } from '../../src/index'; import { setupScratch, teardown } from '../_util/helpers'; import { Fragment } from '../../src'; import { i as ctxId } from '../../src/create-context'; -import options from '../../src/options'; /** @jsx h */ @@ -353,10 +352,6 @@ describe('createContext', () => { }); it('should not re-render the consumer if the context doesn\'t change', () => { - - // Disable the debounce rendering - options.debounceRendering = f => f(); - const { Provider, Consumer } = createContext(); const CONTEXT = { i: 1 }; @@ -402,31 +397,35 @@ describe('createContext', () => { expect(Inner.prototype.render).to.have.been.calledOnce.and.calledWithMatch(CONTEXT, {}, { ['__cC' + (ctxId - 1)]: {} }); expect(scratch.innerHTML).to.equal('
1
'); - render( - - - {data => } - - , - scratch - ); + act(() => { + render( + + + {data => } + + , + scratch + ); + }); // Rendered three times, should call 'Consumer' render two times expect(Inner.prototype.render).to.have.been.calledTwice.and.calledWithMatch({ i: 2 }, {}, { ['__cC' + (ctxId - 1)]: {} }); expect(scratch.innerHTML).to.equal('
2
'); }); - it('should allow for updates of props', () => { - let set; - const MyContext = createContext(); + it.only('should allow for updates of props', () => { + let app; + const { Provider, Consumer } = createContext(); class App extends Component { constructor(props) { super(props); this.state = { x: 'y' }; - set = this.setState.bind(this); + this.renderInner = this.renderInner.bind(this); + + app = this; } renderInner() { @@ -435,19 +434,25 @@ describe('createContext', () => { render() { return ( - - + + {this.renderInner} - - + + ); } } - render(, scratch); + act(() => { + render(, scratch); + }); + expect(scratch.innerHTML).to.equal('

y

'); - set({ x: 'x' }); + act(() => { + app.setState({ x: 'x' }); + }); + rerender(); expect(scratch.innerHTML).to.equal('

x

'); }); From 96fe0ac810d0b3aaa32f8b14bd4ef1bab3ff1229 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Tue, 17 Sep 2019 15:29:16 +0300 Subject: [PATCH 5/7] Remove the .only --- test/browser/createContext.test.js | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index 854a65dacf..09a6c522a2 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -413,7 +413,7 @@ describe('createContext', () => { expect(scratch.innerHTML).to.equal('
2
'); }); - it.only('should allow for updates of props', () => { + it('should allow for updates of props', () => { let app; const { Provider, Consumer } = createContext(); class App extends Component { @@ -469,23 +469,19 @@ describe('createContext', () => { sinon.spy(Inner.prototype, 'render'); - render( - - - {data => } - - , - scratch - ); + act(() => { - render( - - - {data => } - - , - scratch - ); + render( + + + {data => } + + , + scratch + ); + + rerender(); + }); // Rendered twice, with two different children for consumer, should render twice expect(Inner.prototype.render).to.have.been.calledTwice; From 26a28da4d526a29a38e8042d1a7713c7b2a3253d Mon Sep 17 00:00:00 2001 From: cristianbote Date: Wed, 18 Sep 2019 13:26:40 +0300 Subject: [PATCH 6/7] Try to shortcircuit the sCU for Consumer --- src/create-context.js | 3 --- src/diff/index.js | 3 ++- test/browser/createContext.test.js | 26 +++++++++++++++++--------- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/create-context.js b/src/create-context.js index 8c56c47b65..7daae35348 100644 --- a/src/create-context.js +++ b/src/create-context.js @@ -13,9 +13,6 @@ export function createContext(defaultValue) { _id: '__cC' + i++, _defaultValue: defaultValue, Consumer(props, context) { - this.shouldComponentUpdate = function (_props, _state, _context) { - return context !== _context || _props.children === props.children; - } return props.children(context); }, Provider(props) { diff --git a/src/diff/index.js b/src/diff/index.js index dec4a0e0ed..434c63b5fb 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -41,6 +41,7 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi tmp = newType.contextType; let provider = tmp && context[tmp._id]; let cctx = tmp ? (provider ? provider.props.value : tmp._defaultValue) : context; + let isConsumer = provider && tmp.Consumer === newType; // Get component and set it to `c` if (oldVNode._component) { @@ -85,7 +86,7 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi c.componentWillReceiveProps(newProps, cctx); } - if (!force && c.shouldComponentUpdate!=null && c.shouldComponentUpdate(newProps, c._nextState, cctx)===false) { + if (!force && !isConsumer && c.shouldComponentUpdate!=null && c.shouldComponentUpdate(newProps, c._nextState, cctx)===false) { c.props = newProps; c.state = c._nextState; c._dirty = false; diff --git a/test/browser/createContext.test.js b/test/browser/createContext.test.js index 09a6c522a2..72812aa6c8 100644 --- a/test/browser/createContext.test.js +++ b/test/browser/createContext.test.js @@ -420,7 +420,7 @@ describe('createContext', () => { constructor(props) { super(props); this.state = { - x: 'y' + status: 'initial' }; this.renderInner = this.renderInner.bind(this); @@ -428,13 +428,13 @@ describe('createContext', () => { app = this; } - renderInner() { - return

{this.state.x}

; + renderInner(value) { + return

{value}: {this.state.status}

; } render() { return ( - + {this.renderInner} @@ -447,14 +447,14 @@ describe('createContext', () => { render(, scratch); }); - expect(scratch.innerHTML).to.equal('

y

'); + expect(scratch.innerHTML).to.equal('

value: initial

'); act(() => { - app.setState({ x: 'x' }); + app.setState({ status: 'updated' }); + rerender(); }); - rerender(); - expect(scratch.innerHTML).to.equal('

x

'); + expect(scratch.innerHTML).to.equal('

value: updated

'); }); it('should re-render the consumer if the children change', () => { @@ -480,7 +480,15 @@ describe('createContext', () => { scratch ); - rerender(); + // Not calling re-render since it's gonna get called with the same Consumer function + render( + + + {data => } + + , + scratch + ); }); // Rendered twice, with two different children for consumer, should render twice From 5ac8867bf4a8be95fbb010c39d0586254d63dd58 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Thu, 19 Sep 2019 16:00:38 +0300 Subject: [PATCH 7/7] Remove the isConsumer flag since its not needed --- src/diff/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index 434c63b5fb..dec4a0e0ed 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -41,7 +41,6 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi tmp = newType.contextType; let provider = tmp && context[tmp._id]; let cctx = tmp ? (provider ? provider.props.value : tmp._defaultValue) : context; - let isConsumer = provider && tmp.Consumer === newType; // Get component and set it to `c` if (oldVNode._component) { @@ -86,7 +85,7 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi c.componentWillReceiveProps(newProps, cctx); } - if (!force && !isConsumer && c.shouldComponentUpdate!=null && c.shouldComponentUpdate(newProps, c._nextState, cctx)===false) { + if (!force && c.shouldComponentUpdate!=null && c.shouldComponentUpdate(newProps, c._nextState, cctx)===false) { c.props = newProps; c.state = c._nextState; c._dirty = false;