ignore mutation detection for specified branches

[darthrellimnad]

I've been using redux-immutable-state-invariant for a while now and it's been really helpful :)

I ran across the following issue while looking into a way to deal with some performance problems when using this in development with a redux store that houses very large datasets: https://github.com/leoasis/redux-immutable-state-invariant/issues/20

Figured i'd take a stab at it, but let me know if I'm just making a mess :)

My use case doesn't actually use Immutable.js for any branches of state, but we store lots data that we receive from an API as normalized entities under specific paths (using normalizr: https://github.com/paularmstrong/normalizr).

When we need to test locally with large datasets, we found redux-immutable-state-invariant really made our app chug, so we would disable the mutation checking entirely. I desired a way to turn "off" mutation detection via configuration so that we can keep detection enabled for most branches, while ignoring the problematic ones as needed.

These changes add an options argument to immutableStateInvariantMiddleware. The only supported option right now is ignore, which can be an array of dot-separated "path" strings:

// Any branch matching a path string will be skipped and report as "not mutated".
const mw = immutableStateInvariantMiddleware(undefined, {
  ignore: ['entities', 'foo.bar']
})

Some notes on implementation:

• This is kind of an opinionated implementation, so let me know if this seems problematic.
• Root state can not be ignored this way. Using empty string in ignore array won't match any branches of state.
• array indices can be ignored, but doesn't match familiar syntax e.g. 'stuff.0' path string would ignore mutation detection for elt at state.stuff[0]

These changes also alter the function signature of the exported trackForMutations method, which could be a problem if people are using this method directly, outside of normal usage via immutableStateInvariantMiddleware, so this can be considered a breaking change in that respect. Usage of immutableStateInvariantMiddleware should be backwards compatible.

[coveralls]

Coverage remained the same at 100.0% when pulling d629c454a47c4e8adee4c1370d9700a69e4aaa9c on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling d629c454a47c4e8adee4c1370d9700a69e4aaa9c on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 8ed22efd3f63e4b96c42b64b2cee818400ea9351 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 8ed22efd3f63e4b96c42b64b2cee818400ea9351 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[leoasis]

Also some comments about what you said:

Root state can not be ignored this way. Using empty string in ignore array won't match any branches of state. If this is the case, then probably you don't even need this middleware, right? So I'd say it's not a problem.

array indices can be ignored, but doesn't match familiar syntax e.g. 'stuff.0' path string would ignore mutation detection for elt at state.stuff[0]

This is fine for me, though it's a weird use case to try to ignore a specific index in an array.

These changes also alter the function signature of the exported trackForMutations method, which could be a problem if people are using this method directly, outside of normal usage via immutableStateInvariantMiddleware, so this can be considered a breaking change in that respect.

trackForMutations is not exposed in the public API, so we're free to change it. Anyway, I proposed a change in the middleware signature so that will require a major version bump.

[darthrellimnad]

thanks for the feedback! :) I'll get started on those changes soon (have to finish up some other work related things first).

re: ignoring array indices - I agree, uncommon use case and I don't think I'd ever have use for it. It just sort of ended up being supported with the implementation I came up with, so figured I'd call it out :)

[coveralls]

Coverage remained the same at 100.0% when pulling 06d8fb6b952efe0bc1de41b2c220eb147b313f9f on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 06d8fb6b952efe0bc1de41b2c220eb147b313f9f on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 8b76404d7e046dc8f69d5289807b3698f21b2d0a on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[darthrellimnad]

Code review comments have been addressed, so let me know if anything else is desired or necessary :). I can update the README if you'd like to document the public api options, although not sure if you have a preferred format or feel this is necessary at this time.

Edit: also added a test for the isImmutable option, after realizing i hadn't run test:coverage script. that seemed to be the only test case keeping branch coverage under 100%

[coveralls]

Coverage remained the same at 100.0% when pulling aa42219dff02aa8b0ef44f942cdf00ec0c9bc0f4 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage remained the same at 100.0% when pulling aa42219dff02aa8b0ef44f942cdf00ec0c9bc0f4 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[leoasis]

I can update the README if you'd like to document the public api options, although not sure if you have a preferred format or feel this is necessary at this time.

Actually yes! I was going to ask you but forgot. Also, I will have time to review this on Monday probably, so if you can include the readme change before I review that'd be great (no problem if not!)

[coveralls]

Coverage decreased (-0.7%) to 99.296% when pulling a8beac04f7be70cb2a0ca4ddf480d8b6fb5a0e04 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[coveralls]

Coverage decreased (-0.7%) to 99.296% when pulling 985f230e15b2ac4b453295ffc3d233d411d0538c on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[darthrellimnad]

OK, took a stab at some public API documentation :) however, when i pushed the change, looks like the test coverage dropped and is now failing a check. not sure how this change would have affected that, but it may be something I'm missing or not understanding in the CI environment or with coveralls specifically?

Edit: Looked at it a bit more, and may actually be related to a recent version update of coveralls to v2.12.0. Just a guess at the moment, but release notes mention updates for branch coverage support: https://github.com/nickmerwin/node-coveralls/releases

A recent issue also seems similar to what I'm seeing, so maybe it'll be fixed in a patch release or more info will be provided: https://github.com/nickmerwin/node-coveralls/issues/158

This update seems to have been made shortly before my last commit was pushed to origin and failed builds seem to correspond to travis job logs that report installing the updated (v2.12.0) version of coveralls.

[coveralls]

Coverage decreased (-0.7%) to 99.296% when pulling 19462490f0d005a2808281628e35e5b596d4dc94 on darthrellimnad:ignore-branches into bc9aa9824ded63b351e35c67d82082b9833a6c53 on leoasis:master.

[leoasis]

Cool! Thanks for the work on this PR! Will merge it and soon I'll create a new release. Will tweak the readme a bit but I'll do that on my own so that I don't take more time from you.

Again, thanks a lot! 🎉

[darthrellimnad]

np 👍 thanks again for sharing!

[keremciu]

amazing job thanks guys! 🍾 @darthrellimnad @leoasis