Make preferences reducer deterministic

[noisysocks]

Calling Date.now() from the preferences reducer makes the reducer impure, which will bite us if we use any of the time travelling features that Redux enables. To fix this, I've moved the call to Date.now() to the action creator as an optional argument.

This is a small mistake introduced in #5342, and pointed out by @aduth in #5342 (comment).

[aduth]

It's certainly a fair bit more verbose 😬

Initial thoughts are whether we can / should...

• Implement as a middleware augmenting all relevant actions?
• Stop adding positional arguments to action creators, as they're starting to lose meaning, and instead introduce an options object?

Will give a closer look tomorrow.

[noisysocks]

It's probably simpler to just avoid adding an extra argument altogether, so long as we don't mind using an expect.any to unit test insertBlocks and replaceBlocks, e.g.

// In editor/store/actions.js:
export function insertBlocks( blocks, index, rootUID ) {
	return {
		type: 'INSERT_BLOCKS',
		blocks: castArray( blocks ),
		index,
		rootUID,
		time: Date.now(),
	};
}

// In editor/store/test/actions.js:
expect( insertBlocks( blocks, index ) ).toEqual( {
	type: 'INSERT_BLOCKS',
	blocks,
	index,
	time: expect.any( Number ),
} );

[aduth]

I'd be fine with that.

[noisysocks]

Oh... 🤦‍♂️ but that would mean we can't use these action creators (e.g. replaceBlocks()) in our effect tests (e.g. describe( '.MERGE_BLCOCKS')). Not sure that I'm into changing the effect tests to assert against the raw action objects instead of calling action creators, since it's fragile and verbose.

Stop adding positional arguments to action creators, as they're starting to lose meaning, and instead introduce an options object?

I'd be into this.

Will give a closer look tomorrow.

👌

[aduth]

Not sure I'm into letting unit tests force our hand into how best to implement the code under test 😆

[noisysocks]

Very true! I've updated the PR.

[aduth]

Nice 👍

[aduth]

Alternatively, we could do something like:

expect( dispatch ).toHaveBeenCalledWith( {
	...replaceBlocks( /* ... */ ),
	time: expect.any( Number ),
} );

[noisysocks]

Ah, that's nicer 🙂 — updated.