leoasis / redux-immutable-state-invariant

Redux middleware that detects mutations between and outside redux dispatches. For development use only.
MIT License
937 stars 37 forks source link

add stateInvariantTestHelper named export #9

Closed jebeck closed 8 years ago

jebeck commented 8 years ago

Hi again @leoasis. This is the PR I referenced in #8 - the real reason I cloned to work on something and noticed the failing tests.

I first read about this project of yours in the redux docs, and recently it came up again for us at Tidepool. We don't particularly have a use for the middleware, but we love the ability to detect mutations recursively. We are quite obsessed with testing, and one of the things we test in all of our reducer tests is that we're not mutating the initial state (for any reducer that operates on a slice of state that's an object or array). So this PR adds a named export stateInvariantTestHelper providing an interface to track an initial state fixture (trackObj) in a test and then a function (hasMutated) to query for whether the object has been mutated at any level in a test expectation.

We use it like this:

    it('should not mutate object in handling SET_BLIP_VIEW_DATA_URL', () => {
      let initialState = {};
      const tracked = stateInvariantTestHelper.trackObj(initialState);
      let finalState = misc.blipUrls(initialState, {
        type: actionTypes.SET_BLIP_VIEW_DATA_URL,
        payload: actionPayload
      });
      expect(stateInvariantTestHelper.hasMutated(tracked)).to.be.false;
    });

Before I go any further, I wanted to find out if this is a change/PR that interests you since obviously it expands the use cases that this repo would be supporting. If you are interested, I'm happy to discuss the particulars of the new export's interface (I'm not quite sure yet that it's ideal), add some tests, and update the README with the usage. If you're not interested, just let me know, and we at Tidepool will then probably change the name of our fork, remove the middleware-specific code, and publish to npm separately just with the test helper.

leoasis commented 8 years ago

Hey @jebeck thanks again for sending a PR. Unfortunately in this case, I don't think it's worth merging it because it's both out of the scope of the library, and also you can create your own wrapper outside of this lib by just importing the trackForMutations module and wrapping it in the code you just provided.

Having said that, I do think the best solution is to extract the mutation tracking part to another library, and make this one use that function. If you'll be going to do a fork from this code to do that, I'd appreciate if you mention it in a credits section or similar in the repo you'll be creating (not obligated to, but I'd really appreciate it)

jebeck commented 8 years ago

Hi @leoasis, thanks for the quick response! Totally understand about this PR going outside the scope of your project. The reason we don't want to just write a local wrapper is that we already have two client apps using redux (and a third coming soon, probably), so we want something we can import easily across repos.

I do agree that extracting the trackForMutations as a separate utility/library that both this middleware and our test helper would consume as a dependency is the most correct way to do things. If I do that extraction (definitely with full & explicit credit to you!), then I feel like I'll be committing to its maintenance, so I have to think about that a bit.

What I may do in the short term (as we're wrapping up our second redux migration and hoping to deploy it in a matter of weeks) is do the quick thing of changing the package.json in my fork and publishing the test helper only (w/o the middleware piece) to npm under another name like redux-state-invariant-test-helper or similar. I should still be able to pull in updates on my fork if anything changes in trackForMutations, so while it's not ideal, it should suffice for a while.

jebeck commented 8 years ago

(FYI, feel free to close this PR. Would close it myself, but didn't want to make it look like I'm not open to further discussion!)

leoasis commented 8 years ago

Ok sounds good, I'll probably extract the mutation tracker module to its own package soon (when I find the time), but in the mean time what you want to do seems to be enough for your needs

leoasis commented 8 years ago

Closing, but feel free to comment further if you want!

jebeck commented 8 years ago

Ok I've followed you, so I'll try to keep any eye out for the repo if/when you break out the mutation tracker. Thanks again!