Closed TriPSs closed 7 years ago
:wave: @TriPSs thank you for putting this together!
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?
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?
@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
Let me know and i can update the pr
@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.
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
@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`
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 https://github.com/ericclemmons/react-resolver/pull/121#issuecomment-262302221, or mute the thread https://github.com/notifications/unsubscribe-auth/ACZPBYxVn7S1dX__cCWosuVTdgrBd913ks5rAyIXgaJpZM4K2k6- .
@TriPSs why wouldn't you want to overwrite it then?
@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?
Yep... What I propose is that we should not ignore any props... If the developer has written it, he's responsible. :P
@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 :)
@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?
@TriPSs yessir! Does that work for you?
@peterpme Yes! Updated the pull request!
@monder and @salmanm - what do ya say?! Can we merge TriPSs' hard work in?!
@peterpme sure, I like it...
Published v3.1.0 on npm. Thank you all.
This way it does not conflict with mapStateToProps when the prop is still empty.