Skip to content

Commit

Permalink
Remove unused mount check in createContext (-6 B) (#2039)
Browse files Browse the repository at this point in the history
The mount check in Provider sCU was redundant because renderComponent already checks a component is mounted before attempting to rerender it. Further whenever a consumer is unmounted we remove it from the sub list.
  • Loading branch information
andrewiggins committed Oct 28, 2019
1 parent ff9278d commit 5e7997d
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 10 deletions.
45 changes: 45 additions & 0 deletions hooks/test/browser/useContext.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,4 +242,49 @@ describe('useContext', () => {
done();
});
});

it('should not rerender consumers that have been unmounted', () => {
const context = createContext(0);
const Provider = context.Provider;

const Inner = sinon.spy(() => {
const value = useContext(context);
return <div>{value}</div>;
});

let toggleConsumer;
let changeValue;
class App extends Component {
constructor() {
super();

this.state = { value: 0, show: true };
changeValue = value => this.setState({ value });
toggleConsumer = () => this.setState(({ show }) => ({ show: !show }));
}
render(props, state) {
return (
<Provider value={state.value}>
<div>{state.show ? <Inner /> : null}</div>
</Provider>
);
}
}

render(<App />, scratch);
expect(scratch.innerHTML).to.equal('<div><div>0</div></div>');
expect(Inner).to.have.been.calledOnce;

act(() => changeValue(1));
expect(scratch.innerHTML).to.equal('<div><div>1</div></div>');
expect(Inner).to.have.been.calledTwice;

act(() => toggleConsumer());
expect(scratch.innerHTML).to.equal('<div></div>');
expect(Inner).to.have.been.calledTwice;

act(() => changeValue(2));
expect(scratch.innerHTML).to.equal('<div></div>');
expect(Inner).to.have.been.calledTwice;
});
});
11 changes: 2 additions & 9 deletions src/create-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@ import { enqueueRender } from './component';

export let i = 0;

/**
*
* @param {any} defaultValue
*/
export function createContext(defaultValue) {
const ctx = {};

Expand All @@ -25,11 +21,8 @@ export function createContext(defaultValue) {
this.shouldComponentUpdate = _props => {
if (props.value !== _props.value) {
subs.some(c => {
// Check if still mounted
if (c._parentDom) {
c.context = _props.value;
enqueueRender(c);
}
c.context = _props.value;
enqueueRender(c);
});
}
};
Expand Down
50 changes: 49 additions & 1 deletion test/browser/createContext.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ describe('createContext', () => {
app.setState({ status: 'updated' });
rerender();
});

expect(scratch.innerHTML).to.equal('<p>value: updated</p>');
});

Expand Down Expand Up @@ -496,6 +496,54 @@ describe('createContext', () => {
expect(scratch.innerHTML).to.equal('<div>1</div>');
});

it('should not rerender consumers that have been unmounted', () => {
const { Provider, Consumer } = createContext(0);

const Inner = sinon.spy(props => <div>{props.value}</div>);

let toggleConsumer;
let changeValue;
class App extends Component {
constructor() {
super();

this.state = { value: 0, show: true };
changeValue = value => this.setState({ value });
toggleConsumer = () => this.setState(({ show }) => ({ show: !show }));
}
render(props, state) {
return (
<Provider value={state.value}>
<div>
{state.show ? (
<Consumer>{data => <Inner value={data} />}</Consumer>
) : null}
</div>
</Provider>
);
}
}

render(<App />, scratch);
expect(scratch.innerHTML).to.equal('<div><div>0</div></div>');
expect(Inner).to.have.been.calledOnce;

changeValue(1);
rerender();
expect(scratch.innerHTML).to.equal('<div><div>1</div></div>');
expect(Inner).to.have.been.calledTwice;

toggleConsumer();
rerender();
expect(scratch.innerHTML).to.equal('<div></div>');
expect(Inner).to.have.been.calledTwice;

changeValue(2);
rerender();
expect(scratch.innerHTML).to.equal('<div></div>');
expect(Inner).to.have.been.calledTwice;
});

describe('class.contextType', () => {
it('should use default value', () => {
const ctx = createContext('foo');
Expand Down

0 comments on commit 5e7997d

Please sign in to comment.