WIP: Add warnMutations middleware

[leoasis]

Aims to fix #135

I've implemented the mutation warnings as a middleware, which expects to receive plain actions (should be the last in the chain or at least should be after the ones that transform the actions such as the thunkMiddleware).

Still need to provide support for stuff like ImmutableJS but should be straight forward. Also, I'm leaving it like this so that we can discuss implementation.

We can also discuss what's the best way to handle adding this middleware by default on dev, but not in prod (and avoid including the dependencies as well).

[goatslacker]

Copying deep on every dispatch is probably not feasible unless you're building apps with small state.

I can't think of another way to do this off the top of my head though.

[leoasis]

This is meant to be used in development only.

[gaearon]

This assumes dispatch is sync so it needs to be the last in chain right?

[leoasis]

@gaearon yes:

I've implemented the mutation warnings as a middleware, which expects to receive plain actions (should be the last in the chain or at least should be after the ones that transform the actions such as the thunkMiddleware).

[taylorhakes]

A recent pull request removed lodash dependency. I think it would be better to avoid using lodash

[leoasis]

Pushed some updates: Avoiding using lodash and checking for immutable types.

I'm not sure if it's a good thing to avoid using libraries that have been battle tested like lodash in spite of library size. We'll be maintaining and having to support edge cases for deep cloning or whatever we're replacing from lodash. Still, perhaps that's enough for our needs in this case, so if anyone sees something odd here or anything I'm missing let me know! I'll try to add more test cases to see I'm not missing any basic scenario

[gaearon]

We'll be maintaining and having to support edge cases for deep cloning or whatever we're replacing from lodash.

I only think it makes sense to avoid lodash for the core stuff. Plugins (which should be in separate repos anyway) can use lodash, especially if they're dev-only.

[acdlite]

If/when you do use lodash, you should also require the specific module you want to use.

// This
import isEqual from "lodash.isequal"

// instead of this
import isEqual from "lodash/lang/isEqual"

[leoasis]

If/when you do use lodash, you should also require the specific module you want to use.

@acdlite what's the reason for that? the latter option will still make the bundle require only the needed modules and no more

[gaearon]

the latter option will still make the bundle require only the needed modules and no more

This is correct AFAIK.

[acdlite]

True, but the first way it makes it less likely another dev will see a lodash dependecy and be tempted to import the entire library elsewhere.

[johanneslumpe]

To me personally there is no difference between the two versions, except that the latter is more descriptive. If at all, the second one would be more logical to me, as people see that you are specifically importing a subset/function directly, instead of a prop from the whole lib.

Sent from my iPhone

On 20 Jun 2015, at 02:28, Andrew Clark notifications@github.com wrote:

True, but the first way it makes it less likely another dev will see a lodash dependecy and be tempted to import the entire library elsewhere.

—
Reply to this email directly or view it on GitHub.

[acdlite]

Think about what the corresponding package.json files look like. The first has lodash.isequal and the second has lodash. Another developer may see a dependency on lodash and assume its safe to import the whole library, whereas a dependency on lodash.isequal forces restraint. In other words, the first version makes it impossible for an errant import statement to dramatically increase the size of the library; you'd have to also update package.json.

Clearly this is a matter of preference, though.

[leoasis]

I agree it's mostly a matter of preference. The only concern I have is that
it is very likely that the end user will already be using lodash (compared
to using the separate function modules), and if we depend on just the
function module we will be making the final user bundle even larger since
it will contain duplicated stuff that webpack or any other bundler won't be
able to realize it's the same.

But again since this particular thing is just for development, that
argument perhaps it's not that important.

On Saturday, June 20, 2015, Andrew Clark notifications@github.com wrote:

Think about what the corresponding package.json files look like. The
first has lodash.isequal and the second has lodash. Another developer may
see a dependency on lodash and assume its safe to import the whole library,
whereas a dependency on lodash.isequal forces restraint. Clearly this is
a matter of preference, though.

—
Reply to this email directly or view it on GitHub
#138 (comment).

[acdlite]

Huh, I never import the entire lodash module ¯_(ツ)_/¯

[leoasis]

Not meant import, but have lodash as a dependency in package.json

On Saturday, June 20, 2015, Andrew Clark notifications@github.com wrote:

Huh, I never import the entire lodash module ¯_(ツ)_/¯

—
Reply to this email directly or view it on GitHub
#138 (comment).

[leoasis]

In any case, since the argument I mentioned is not strong in this case
since this middleware is meant to be used in development only, let's go
with the function module dependency

On Saturday, June 20, 2015, Leonardo Andrés Garcia Crespo leoasis@gmail.com
wrote:

Not meant import, but have lodash as a dependency in package.json

On Saturday, June 20, 2015, Andrew Clark <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Huh, I never import the entire lodash module ¯_(ツ)_/¯

—
Reply to this email directly or view it on GitHub
#138 (comment).

[leoasis]

Ok I think this is now mostly complete. We now need to decide if we'll leave it here, move it to a different repo, and if leaving it here if we should automatically add it to the default redux middleware if we detect we're in the development env (assuming process.env.NODE_ENV !== 'production'?).

[gaearon]

Please can you move this to a separate repo and try to get it working with the breaking-changes-1.0 branch? I'm closing for now, as it doesn't belong in the core.

[leoasis]

@gaearon will do! Any particular name for this one you would prefer? redux-warn-mutations? redux-dev-warn-mutations (to make it clear that it's just for dev)?

[0xR]

@leoasis's library is here in case you are looking for it in this thread:
https://github.com/leoasis/redux-immutable-state-invariant

[leoasis]

@0xR thanks for adding this here