Lifecycle design changes (persistent component instances) #130

slimsag · 2017-08-07T02:29:30Z

Fixes #115

Prior to this change a component's instance would be outright replaced if the parent rendered a new component pointer; now instead of doing that, we follow the react-like approach of keeping the old component pointer (the one first rendered) as the persistent component instance. The parent properties are copied over using reflection. See #115 (comment) for further details.

…tances)

…ances)

…e_SkipRender

codecov-io · 2017-08-07T02:42:15Z

Codecov Report

Merging #130 into master will decrease coverage by 3.69%.
The diff coverage is 39.58%.

@@            Coverage Diff            @@
##           master     #130     +/-   ##
=========================================
- Coverage   64.72%   61.02%   -3.7%     
=========================================
  Files           5        5             
  Lines         309      331     +22     
=========================================
+ Hits          200      202      +2     
- Misses         97      116     +19     
- Partials       12       13      +1

Impacted Files	Coverage Δ
dom.go	`65.14% <39.58%> (-5.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74dd2fa...8077c96. Read the comment docs.

slimsag · 2017-08-07T02:43:02Z

Also see this branch which has one additional commit, which I don't intend to merge (for obvious reasons). It's a test / example file I used for developing this: https://github.com/gopherjs/vecty/tree/lifecycle-test

slimsag · 2017-08-07T02:45:39Z

dom.go

-	comp.Context().prevComponent = comp
-	comp.Context().prevRenderComponent = doCopy(comp)
+	//
+	// TODO: In the event that this really is the previous component instance,


I'm like 80% certain this is the cause of the issue mentioned in this TODO comment of my test file: 7fc6935#diff-59654adfef9097fef9e9fede9315019aR45

The overarching problem here is that, now that we have persistent component references, it's actually not always obvious where the proper Context() really is. Someone will need to sit down and rethink how to fix this issue here + hopefully make the Context storage location a bit easier to think about..

I suspect the behaviour that the comment refers to is a bug I'm already aware of, where a rerender gets queued for a child component that in the mean time gets removed or replaced in the DOM by a parent rerender. If this is indeed the same bug, it will be fixed by lifecycle event changes (mount/unmount) that I already have in the pipeline.

Actually I don't think that is it that. The bug here is that:

Imagine a two-component setup: a parent, and its child.

Rerender(thePrevParent) is called, where thePrevParent is the 'previous' component instance (one the user has a pointer to).

renderComponent(thePrevParent, ...) will be called and so the 'previous' component instance (thePrevParent.Context()) will be updated on lines 467-469 below.

When its children are walked, it will effectively invoke renderComponent(theNextChild, thePrevChildContext, thePrevChildRender), with theNextChild being the newly created component instance -- NOT the persistent / previous component instance. So the 'next' component instance (theNextChild.Context()) will be updated on lines 467-469.

To be entirely honest, this is hard for me to reason about, and that's part of the problem^. The troublesome bit here is that... it's not clear whether or not the 'next' or 'previous' Component.Context has the information and which needs to be updated.

I'll play around with this some more to see if I can resolve this issue / remove the ambiguity..

Ok, fixing the above actually turned out to be quite easy. I just needed to make the persistent component instance always be the source of truth for Context information.

I've done this in df133f3 and it fixes that TODO (see e391b1b)

slimsag · 2017-08-07T02:49:53Z

@pdf here is what I've managed to get. It's EOD Sunday for me, and I'm not sure when I'll have time again to dive into Vecty code (probably a few weeks).

In an effort to not block you from making changes, I suggest:

If this looks mostly good to you, I can merge it and you can work from there.
If you think it needs more work before merging, I can close this PR and you can create a new one based on this one.
If you think this is all wrong for some reason, or that there's a better approach to take, I can close and you can create a brand new PR. :)

Just let me know which you prefer ^^

pdf · 2017-08-07T06:47:51Z

Will be next weekend before I can review and make a determination, thanks @slimsag

…nt component instance) This fixes the one bug mentioned at #130 (comment)

slimsag · 2017-08-12T19:12:17Z

Sorry for the last-minute change BTW, hope that isn't too confusing. ^^ let me know what you think.. I'm all done making changes now..

pdf · 2017-08-13T12:33:53Z

No problem, however I think there are some bad assumptions made, particularly in the latest commit - I don't believe it accounts for the possibility that a child may change types between renders, and there are no checks for component type equality.

I've taken your commits that clean up the Restore-related code, and your copyProps func, plus some of your improved comments into an implementation that I believe is a little simpler, and more accurate in #131. I would certainly like to add some tests though.

Let me know what you think, and we can continue either here or there, based on your thoughts.

Changes imported via: ``` git diff lifecycle..pdf/persistent_component_lifecycle > pdf.patch git checkout lifecycle && git apply pdf.patch git checkout -- doc/ # keep PR reference number correct ``` I then made the following changes: 1. Rename `Core.prevComponent` -> `Core.prevRenderComponent` once again in `dom.go`. 2. `git checkout -- dom_test.go` <- renaming `prevComponent` to `prevRenderComponent` once again. 3. Exclude change in `markup.go` (will send separate PR to discuss this further).

pdf · 2017-08-14T08:44:03Z

I wasn't quite ready for this to be merged to master, as there are some outstanding things to discuss, mostly around what I'll cover in #132.

slimsag · 2017-08-15T05:19:32Z

@pdf apologies for the premature merge, I figured that me and you both felt pretty confident about this implementation and, even if not complete due to things like #132, merging would be OK given Vecty hasn't had a 1.0 release yet. I should have waited for your go ahead, though, I'll do that next time! If desireable, we can temporarily revert out of master.

pdf · 2017-08-15T05:25:54Z

No problem, I should have been explicit about my opinion of the code state. My concern is mostly for people trying to take Vecty for a spin and running into the inconsistent and hard to diagnose behaviour that this change may exhibit currently.

dmitshur · 2017-09-19T23:07:47Z

The changelog says:

Properties should be denoted via `vecty:"prop"` struct field tags.

What are properties and what are non-properties? How do I know if a field should be marked with that struct field tag?

pdf · 2017-09-19T23:32:20Z

Fields that are updated from the parent need to be tagged:

type InnerComponent struct {
  vecty.Core
  ParentSet bool `vecty:"prop"`
  localState bool
}

func (c *InnerComponent) Render() *vecty.HTML {
  c.localState = !c.localState
  return elem.Div(
    vecty.If(c.ParentSet, elem.Span(`true updated from parent`)),
    vecty.If(c.localState, elem.Span(`true updated locally`)),
  )
}

type OuterComponent struct {
  vecty.Core
  flip bool
}

func (c *OuterComponent) Render() *vecty.HTML {
  c.flip = !c.flip
  return elem.Div(
    &InnerComponet{ParentSet: c.flip},
  )
}

@slimsag

This is needed to have correct rendering behavior after the breaking API change of hexops/vecty#130. All of these field are properties that are set by external components, they are not internal state of the component itself. See hexops/vecty#149 (comment): > anywhere a Component is instantiated inside Component.Render(), > with fields set from the parent, those fields need to be tagged > with `vecty:"prop"`. And hexops/vecty#130 (comment): > Fields that are updated from the parent need to be tagged: > > type InnerComponent struct { > vecty.Core > ParentSet bool `vecty:"prop"` > localState bool > } > > func (c *InnerComponent) Render() *vecty.HTML { > c.localState = !c.localState > return elem.Div( > vecty.If(c.ParentSet, elem.Span(`true updated from parent`)), > vecty.If(c.localState, elem.Span(`true updated locally`)), > ) > } > > type OuterComponent struct { > vecty.Core > flip bool > } > > func (c *OuterComponent) Render() *vecty.HTML { > c.flip = !c.flip > return elem.Div( > &InnerComponet{ParentSet: c.flip}, > ) > } Helps hexops/vecty#149. Follows e7b1bc5. /cc @slimsag @pdf

Fix hexops#174 This interface has been removed in PR hexops#130, it shouldn't be used anymore.

Fix #174 This interface has been removed in PR #130, it shouldn't be used anymore.

slimsag added 7 commits August 6, 2017 00:47

remove Restorer interface

9e7acbe

remove Core.prevComponent (not used by anything currently)

21d62b1

correct render description

d4940a3

example/markdown: update to reflect PR #130 (persistent component ins…

fada4c1

…tances)

example/todomvc: update to reflect PR #130 (persistent component inst…

e851c9b

…ances)

update tests for Restore removal; remove TestRenderBody_Restore_Befor…

b536fe5

…e_SkipRender

slimsag force-pushed the lifecycle branch from 54752dd to b536fe5 Compare August 7, 2017 02:40

slimsag commented Aug 7, 2017

View reviewed changes

slimsag mentioned this pull request Aug 7, 2017

Design: Determine how to manage component lifecycle #115

Closed

slimsag added a commit that referenced this pull request Aug 7, 2017

doc: mention PR #130 in CHANGELOG

e5b36c4

doc: mention PR #130 in CHANGELOG

f85cef7

slimsag added 2 commits August 12, 2017 12:07

make Context() single source of truth more clear (always the persiste…

df133f3

…nt component instance) This fixes the one bug mentioned at #130 (comment)

remove prevComponent checks from tests (field removed)

782a1e7

pdf mentioned this pull request Aug 13, 2017

Lifecycle design changes (2) (persistent component instances) #131

Closed

slimsag added 5 commits August 13, 2017 16:58

rename assertHTML -> extractHTML

816abb4

rename matchComponent -> sameType

779ffb2

rename prev/nextComponent -> prev/nextChildComponent

4fe7ced

add comment describing why h.children[i] = prevChild

8077c96

slimsag merged commit ef519fb into master Aug 14, 2017

slimsag deleted the lifecycle branch August 14, 2017 06:57

dmitshur mentioned this pull request Sep 19, 2017

ensure we've arrived at the right API conclusions by confirming in a real-world app like Go-Package-Store #149

Open

dmitshur mentioned this pull request Oct 10, 2017

Port to new Vecty API shurcooL/Go-Package-Store#84

Merged

tbruyelle pushed a commit to tbruyelle/vecty that referenced this pull request Nov 22, 2017

example: remove usage of the Restorer interface

059e5af

Fix hexops#174 This interface has been removed in PR hexops#130, it shouldn't be used anymore.

tbruyelle mentioned this pull request Nov 22, 2017

example: remove usage of the Restorer interface #176

Merged

slimsag pushed a commit that referenced this pull request Nov 23, 2017

example: remove usage of the Restorer interface (#176)

732308a

Fix #174 This interface has been removed in PR #130, it shouldn't be used anymore.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lifecycle design changes (persistent component instances) #130

Lifecycle design changes (persistent component instances) #130

slimsag commented Aug 7, 2017 •

edited

Loading

codecov-io commented Aug 7, 2017 •

edited

Loading

slimsag commented Aug 7, 2017

slimsag Aug 7, 2017

pdf Aug 12, 2017

slimsag Aug 12, 2017

slimsag Aug 12, 2017

slimsag commented Aug 7, 2017

pdf commented Aug 7, 2017

slimsag commented Aug 12, 2017 •

edited

Loading

pdf commented Aug 13, 2017

pdf commented Aug 14, 2017

slimsag commented Aug 15, 2017

pdf commented Aug 15, 2017

dmitshur commented Sep 19, 2017

pdf commented Sep 19, 2017 •

edited

Loading

Lifecycle design changes (persistent component instances) #130

Lifecycle design changes (persistent component instances) #130

Conversation

slimsag commented Aug 7, 2017 • edited Loading

codecov-io commented Aug 7, 2017 • edited Loading

Codecov Report

slimsag commented Aug 7, 2017

slimsag Aug 7, 2017

Choose a reason for hiding this comment

pdf Aug 12, 2017

Choose a reason for hiding this comment

slimsag Aug 12, 2017

Choose a reason for hiding this comment

slimsag Aug 12, 2017

Choose a reason for hiding this comment

slimsag commented Aug 7, 2017

pdf commented Aug 7, 2017

slimsag commented Aug 12, 2017 • edited Loading

pdf commented Aug 13, 2017

pdf commented Aug 14, 2017

slimsag commented Aug 15, 2017

pdf commented Aug 15, 2017

dmitshur commented Sep 19, 2017

pdf commented Sep 19, 2017 • edited Loading

slimsag commented Aug 7, 2017 •

edited

Loading

codecov-io commented Aug 7, 2017 •

edited

Loading

slimsag commented Aug 12, 2017 •

edited

Loading

pdf commented Sep 19, 2017 •

edited

Loading