Duck-type subscription in prop checks

[jquense]

Use PropTypes.shape instead of instanceOf checks which don't work across multiple instances of react-redux. Granted it's not a problem that should occur since they should be deduped, but dependency management is hard, and sometimes that's not feasible due to symlinks and the like. This is (I think) more idiomatic JS anyway :)

thanks! (sorry if something obvious is off i wrote this in the GH editor)

[jquense]

oops my membership in the react org apparently gives me push access here, sorry for the extra branch. I'm not sure why it didn't fork it.

[gaearon]

Yes please, we should never use instanceof.

[jimbolla]

I disagree. This is a helpful warning that people have a broken build process. Since it's a propType validation, it only applies to dev builds and doesn't affect production code.

[jquense]

I see why it may be helpful to some folks, but I don't think its react-redux's job to be prescribing when someone should or should not include multiple instances of the package. The package isn't singleton, and works fine included multiple times, RR doesn't have the context to be warning someone about suboptimal bundling.

If it does want to do that, then the warning should be about multiple instances of react-redux not the far more unclear "This isn't a Subscription (even tho it is but fails a check due to an idiosyncrasy with how instanceof works in JS)".

I don't agree that its help though, as in our case the situation is perfectly expected and only present in DEV.

[gaearon]

This is a helpful warning that people have a broken build process.

The problem is the warning isn't actionable to anyone who doesn't know how instanceof works across packages. These method names also don't tell anything to end users. So even people who could benefit from a fix would brush it off as a weird unactionable noise in the log (and as a result pay even less attention to warnings coming from React).

I also think there are other cases where this could break (for example, iframes have different contexts). I can't easily describe a case like this right now but I wouldn't be surprised if it was plausible.

[jimbolla]

@jquense For consistency, can you change the one in Provider as well?

[timdorr]

@jimbolla Took care of that. Put our shape definitions in one spot.

[timdorr]

Can I get a 👍 on this? I'll merge and then we can release a 5.0.3.

[markerikson]

LGTM

[jquense]

thanks y'all

[nkbt]

Wow, one less library that suffers from npm link/lerna bootstrap issue. Thanks!

In case you have similar issue for other libraries - here is our research/solution on this glenjamin/transit-immutable-js#32

[AlanFoster]

@jquense Out of interest, what scenarios would the instanceof check fail?

Is it common to have multiple dependencies on react-redux? Or is this an issue with transitive dependencies?

[nkbt]

@AlanFoster when you use npm link or lerna or any other solution that would create symlink. This way you end up having different multiple instances and either will be used, depending on how deep your current code is (node resolves "depth first"). So instanceof fails.