Preact: While testing shouldComponentUpdate it seems that the this.props.children never equals nextProps.children

[tkh44]

I was doing some testing on https://github.com/tkh44/shallow-compare and it seems that shouldComponentUpdate will always fail based on props being different. The property children is always different.

I may be missing something or doing something wrong in my tests, but I felt like I should ask about it.

I created a branch with a logging so the issue could be seen here: https://github.com/tkh44/shallow-compare/tree/testing-preact-children. It is set up just like most of preact's family of repos so just npm install and npm t.

Here is what i'm getting in the logs

render red

prevProps {"color":"red","children":["test"]}
nextProps {"color":"blue","children":["test"]}
color is not equal value a: red value b: blue
propsDiffers true
stateDiffers false
shouldComponentUpdate true
color is not equal value a: red value b: blue
propsDiffers true
stateDiffers false
render blue

prevProps {"color":"blue","children":["test"]}
nextProps {"color":"blue","children":["test"]}
children is not equal value a: [ 'test' ] value b: [ 'test' ]
propsDiffers true
stateDiffers false
shouldComponentUpdate true
children is not equal value a: [ 'test' ] value b: [ 'test' ]
propsDiffers true
stateDiffers false
render blue

[developit]

Hmm - yes this is definitely true, though it's sortof by design. As of preact 6.3.0, props.children is always an Array, and each render it'll always be a fresh Array. Previous to 6.3.0, props.children would still have been different on every render when there were more than 0 children.

Not sure what to do here. Does it make sense to special-case .children in the shallow comparison?

[tkh44]

I was wondering the same thing. I would like to keep shallow-compare library agnostic so I'll have to do some testing with it on other libs. I'm particularly worried about devs using any sort of shallowEquals utility in shouldComponentUpdate are not getting expected results.

Would this mean that the PureComponent implementation in preact-compat is not working as expected either?

[developit]

It would appear to be bypassed by this, yes.

[tkh44]

I just added tests for React as well and they fail as well. Not sure what to make of this.

[developit]

@tkh44 do they pass for single children but fail for multiple? That'd be my guess. Their PureComponent implementation doesn't seem to account for this case either.

[tkh44]

@developit correct. If the component is the only child shallow comparing works as expected, but not multiple children.

[developit]

Honestly I don't think very many people would be aware of this. I'm leaning towards special-casing children in preact-compat.

[tkh44]

Would making children be undefined instead of an empty array when there are no children be a crazy idea?

(Edit) I guess that would break when trying to call array functions on children that is undefined.

[developit]

@tkh44 not crazy at all, that's actually what Preact did prior to 6.3.0. Your point about the Array stuff is exactly why it is now always an Array 👍

I think even for components with single children, that .children value is going to be a new VNode every time... kinda a hard thing to deal with.

[developit]

@tkh44 I had a thought: what about having all VNode's with no children share a reference to an empty Array?

[tkh44]

@developit thats so crazy that it might work. I can't think of any negatives at the moment.

[developit]

I keep going back and forth on this 😞
The empty Array thing would fix the case where there are no children, but the equally important case of single same child would still by bypassing. Though - now that I think of it, a child will never be equal across renders because VNodes are nearly always created anew during rendering of the parent....

Honestly this kinda seems like an issue with the whole idea of pure rendering...

[tkh44]

I agree. I don't think there is anything else to be done. Thanks for taking the time to look into this though.

[developit]

I'll probably do a poll and get a sense of whether it's worth dropping the empty array. It's a little bit more code for component authors, but maybe a worthy trade-off. We shall see!

[developit]

Might close this out for now since I don't think we can really go back on the children:[] thing, but I'd love to chat about a workaround in shallow-compare if you have time. Just hit me up in Gitter.

[tkh44]

Sounds good.

[Download]

So I arrived here because I'm hitting a very similar issue.

I'm using preact with jsdom (this is in a test) and what I'm seeing is that the nextProps arguments passed to shouldComponentUpdate contains a children property that is an empty array, whereas this.props does not contain a children prop at all... Is this expected?

I'm on v7.1.0 btw.

[developit]

@Download Use 7.2 - it has a shared children array that should fix the issue.

[Download]

npm install preact

always seems to give me 7.1... is that right?
How to get 7.2?

[developit]

@Download 7.2 was only released under the beta dist-tag. When you do npm i preact you are only ever going to get latest dist-tag releases. I just rolled 7.2 over to latest though, so npm i preact should now give you 7.2 :)

[Download]

Ha that explains it! Thanks Jason!

[Download]

@developit
Still getting 7.1 today:

c:\ws\preact-helmet>npm install --save preact
[email protected] c:\ws\preact-helmet
`-- [email protected]

[Download]

I also see it is still under pre-release label here:
https://github.com/developit/preact/releases

[developit]

Ack! I messed up and the dist-tag change hadn't gone through. It's there now and I updated the releases page.