setState now creates new versions instead of mutating state

[mitranim]

Addresses #1168.

[coveralls]

Coverage remained the same at 100.0% when pulling 576c891 on Mitranim:master into b8a117a on developit:master.

[dandv]

This does fix my #1169 test case. @developit, please merge as soon as you get a chance, and publish a new package version, since this fixes a really insidious class of bugs.

[gamebox]

Could I suggest also including a test of the function parameter version of the function. It looks like you are passing prev by reference here, so wouldn't any changes to it also mutate prev? It would be interesting to explore the effects of that.

[mitranim]

Good suggestion, I just feel like a full setState test suite is somewhat out of scope for the PR. Strange that it's missing though.

Yes it passes the previous state by reference, exactly as it should. Treating the previous state as immutable is the responsibility of the user.

[KevinDoughty]

getDerivedStateFromProps in src/vdom/component.js renderComponent() also mutates state, you might want to create a test that implements it.

[KevinDoughty]

This test still fails:

	it('getDerivedStateFromProps should not mutate state', () => {
		const stateConstant = {};
		class Stateful extends Component {
			constructor() {
				super(...arguments);
				this.state = stateConstant;
			}
			static getDerivedStateFromProps(props,state) {
				return { key: 'value' };
			}
			componentDidMount() {
				expect(stateConstant).to.deep.equal({});
			}
		}
		render(<Stateful />, scratch);
	});

[mitranim]

@KevinDoughty Good catch! Added a similar change for getDerivedStateFromProps, with a test. Now it also creates new versions instead of mutating the state.

[dandv]

Should also add tests for nested props, given this linkState bug.

[mitranim]

@dandv Are you sure the bug still reproduces with this PR?

[dandv]

@mitranim: the linkState bug? I haven't tried to repro it with this PR because linkState works with React as well, and I've just confirmed that the bug reproduces with React too. I was just suggesting adding tests for nested props since it's a tricky enough situation that linkState was tripped by it.

[developit]

I think we're good to merge this. Thanks for the tests and comment updates too, makes it feel a bit safer! :)

[marvinhagemeister]

@mitranim Awesome, thanks for your PR 🎉