Ensure properties and children are 'reset' each render for a projector #501
Conversation
| @@ -215,14 +218,22 @@ export function ProjectorMixin<P, T extends Constructor<WidgetBase<P>>>(base: T) | |||
| } | |||
|
|
|||
| public setChildren(children: DNode[]): void { | |||
| this._projectorChildren = [ ...children ]; | |||
There was a problem hiding this comment.
Why wouldn't we be doing super.__setChildren__(this._projectorChildren)? Feels like a little bit of indirection. Also if we did this, wouldn't we just be able to this.setChildren(this.children) and this.setProperties(this.properties) below?
There was a problem hiding this comment.
We are taking a copy at the point in time the user has explicitly called setProperties or setChildren, because the properties and children that are passed can be decorated in lifecycle phases such as beforeRender - on a re-render for all other widgets in the widget tree a "fresh" properties object is used via the usage of w() but for the projector that is not the case and therefore the next render the possibly modified properties are used again (which means it can cause unexpected behaviour). So with these "copied" local value we re-set the properties/children in WidgetBase by calling the setters.
We could actually directly call the supers __setProperties__ and __setChildren__ from the __render__ method as we only need the to take the copy when the user has actually called the methods intended for public consumption setProperties and setChildren - I can change that.
There was a problem hiding this comment.
Ha nope of course I'm wrong, I do need to take the copy every time over wise my "frozen" properties/children will possibly be mutated then next time they are used to call __setProperties__ or __setChildren__. Doh.
| super.__setChildren__(children); | ||
| } | ||
|
|
||
| public setProperties(properties: P & { [index: string]: any }): void { | ||
| this._projectorProperties = assign({}, properties); |
There was a problem hiding this comment.
Could also do { ...properties } now, though I guess with offloading to native, assign will always be quicker.
There was a problem hiding this comment.
I actually prefer the spreading but you get an error Spread types may only be created from object types. I think when the type is a generic.
There was a problem hiding this comment.
Hmmm... I couldn't find anything on TypeScript issues, so I opened this: microsoft/TypeScript#15792
There was a problem hiding this comment.
It was a dupe... PR microsoft/TypeScript#13288 will resolve it.
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
==========================================
+ Coverage 97.02% 97.04% +0.02%
==========================================
Files 18 18
Lines 907 915 +8
Branches 140 142 +2
==========================================
+ Hits 880 888 +8
Misses 3 3
Partials 24 24
Continue to review full report at Codecov.
|
Type: bug
The following has been addressed in the PR:
Description:
Ensure properties and children are reset each render back to the last received set by either
setPropertiesorsetChildrenResolves #498