Accept props that are empty (For Redux mapStateToProps)

[TriPSs]

This way it does not conflict with mapStateToProps when the prop is still empty.

[peterpme]

👋 @TriPSs thank you for putting this together!

[monder]

Thanks for the PR. As mentioned in #119 - I'm still not sure if just by doing !props[name] won't break anything. Unfortunately we don't have the tests for that.
Maybe we should check for some falsy values explicitly such as null or undefined, but allow false, 0, "" as totally adequate resolve values.
Also, whats the reason for !props[name].length? Is it for empty arrays?

[TriPSs]

Yes that is indeed for empty arrays and correct me if i'm wrong this also works if its an empty string.

And is !props[name] not almost the same as hasOwnProperty?

[monder]

@TriPSs empty strings are already false in javascript.

"" == false && '' == false // true

Alright, I've looked at the code more closely and #119.
Now I also don't see the point of checking the props.hasOwnProperty at all. Correct me if i'm wrong, but the only case for this check is when the prop comes from higher order components. But we still pass all the props in render() - from state and parent props. @salmanm was right then.

I've checked the cases of redux-like stuff and stacked @resolve decorators. All seem to work well without the check at all as well as an example app here.
If that check is indeed a blocker for several people - I think we can get rid of it and keep only

if (!nextState.resolved.hasOwnProperty(name)) {

@ericclemmons What do you think?

/cc @ericclemmons, @TriPSs, @peterpme, @salmanm

[TriPSs]

Let me know and i can update the pr

[TriPSs]

@monder I just toughed, if you remove it completely and the prop is already filled, then it will overwrite it, right? I don't think thats what you want.

[peterpme]

Thank you both for checking in on the issue!

I'd just like to point out that there's a fundamental difference in Javascript (and React) between null and undefined.

defaultProps will not work with null. React expects undefined. I don't completely understand this issue yet, just wanted to keep this in mind when handling truthy / falsey statements

[peterpme]

@TriPSs !props[name] may not be the same because this is strictly checking the value and not the key.

hasOwnProperty will make sure that the object in question has the key you're checking against.

For example:

const someObj = {
  foo: 1,
  bar: null,
  baz: undefined,
}

`someObj.hasOwnProperty(foo) === true`
`someObj.hasOwnProperty(bar) === true`
`someObj.hasOwnProperty(baz) === true`

`!someObj.foo === false`
`!someObj.baz === true`
`!someObj.bar === true`

[salmanm]

To me resolver facilitates an excellent way to "get async stuff loaded
before we render". I would like it to only handle that and give me plumbing
for the same. Now whether I use that to fetch new data or send an existing
data or even return something completely new, should not be a concern of
resolver.

Am I making any sense here?

On Nov 22, 2016 22:42, "Peter Piekarczyk" notifications@github.com wrote:

Thank you both for checking in on the issue!

I'd just like to point out that there's a fundamental difference in
Javascript (and React) between null and undefined.

defaultProps will not work with null. React expects undefined. I don't
completely understand the issue yet, just wanted to keep this in mind.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#121 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACZPBYxVn7S1dX__cCWosuVTdgrBd913ks5rAyIXgaJpZM4K2k6-
.

[monder]

@TriPSs why wouldn't you want to overwrite it then?

[peterpme]

@salmanm I agree, I think that's the issue we're trying to solve and this shouldIgnoreProps is the issue.

@monder it's time I ask - (still getting comfortable with the lib) - are we talking about local props being overwritten by what's coming in from an async request, or making sure we're not making the same request on the client if it's been fulfilled successfully on the server?

[salmanm]

Yep... What I propose is that we should not ignore any props... If the developer has written it, he's responsible. :P

[peterpme]

@TriPSs what do you think? Is that OK? If we remove that last part and roll with the punches?

Thank you for helping out with this by the way, you've made my life a little easier :)

[TriPSs]

@peterpme Oke just to be clear, we are gonna change it to this?
if (!nextState.resolved.hasOwnProperty(name)) {

If so then i will update my pull request.

@monder I thought that when you do that the client wouldn't now it was set, but thats where the nextState.resolved is for right?

[peterpme]

@TriPSs yessir! Does that work for you?

[TriPSs]

@peterpme Yes! Updated the pull request!

[peterpme]

@monder and @salmanm - what do ya say?! Can we merge TriPSs' hard work in?!

[salmanm]

@peterpme sure, I like it...

[monder]

Published v3.1.0 on npm. Thank you all.