Render new #107

pdf · 2017-05-10T08:44:06Z

Refactor rendering for reliability and performance.

NEW:

Keyer interface provides support for efficient list element re-use.
Components simply need to implement the Key function, or the
vecty.Key Markup may be used for HTML.

BREAKING:

Restorer interface is now Updater, and its Restore method is now
ShouldUpdate. The previous Restorer implementation was variously
broken in any case, and poorly named IMO.

Additionally, it requires a Clone method to support passing a
previous Component instance to ShouldUpdate. There is a limited
generic version available on vecty.Core using reflection that may
satisfy for users that do not wish to implement it themselves.
Context method on Component interface is no longer public.
Restore method on HTML is no longer public.

Supersedes: #93 #94 #104
Impacts: #84 #75 #54
Fixes: #106 #92 #85 #78 #25

dmitshur · 2017-05-11T02:49:52Z

dom.go

-// core/central struct which all Component implementations should embed.
+var (
+	errMissingParent = fmt.Errorf("Missing parent node")
+	errEmptyElement  = fmt.Errorf("Empty element or node")


Minor Go style issue, error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context.

Source: https://github.com/golang/go/wiki/CodeReviewComments#error-strings.

dmitshur

Implementing Keyer will provide a significant performance boost when rendering siblings that may change order.

Ooh, that's very interesting. I'll want to play with it for Go Package Store (i.e., issue #92).

dmitshur · 2017-05-11T02:51:40Z

dom.go

+}
+
+// Keyer is an optional interface that Components may implement to uniquely
+// identify an instance amongst a list of its siblings.  Implementing Keyer


Another minor style issue. Go style uses a single space between sentences.

Source: https://dmitri.shuralyov.com/idiomatic-go#single-spaces-between-sentences.

dmitshur · 2017-05-11T03:14:02Z

dom.go

-// core/central struct which all Component implementations should embed.
+var (
+	errMissingParent = fmt.Errorf("missing parent node")
+	errEmptyElement  = fmt.Errorf("empty element or node")


Also, minor, since these are static errors and you're not using any %v verbs, you could use the simpler errors.New instead of fmt.Errorf.

pdf · 2017-05-11T03:29:26Z

We may be able to make an additional improvement by reducing reflows using a single append to the DOM via a DocumentFragment, but I need to work out how best to handle deferring lifecycle (mount/unmount) events there, since they need to be triggered for children after the DOM change. Also need to determine which cases this is optimal for, but should be a safe bet for new elements at a minimum.

I probably won't tackle this strategy for this PR though.

emidoots · 2017-05-11T19:49:12Z

Thanks for sending this. I'm on a vacation for two weeks (since Monday), so I have more spare time. I should have time to merge these changes soon. I'll respond to your other comment #106 (comment) here for better context.

I realise that you're keen to get some tests in before accepting PRs, and I understand that you've had little time to work on this project recently, however I'd like to get something more robust working than what we currently have so that I can move on with my project.

I totally understand your desire to push this forward. I'll be doing my best very soon (hopefully today) to take a look at these changes one-by-one and add tests for them while doing so. I should point out that it is hard to work on such a large PR as this, so I'll be personally breaking up your changes into smaller segments and referencing this PR. I'll keep your git author data attached, though, so it doesn't look like I am stealing your work :P I hope that's alright!

Although I would certainly have preferred smaller more scoped PRs, I just wanted to say thanks again for doing this! Your work was not a wasted effort, and I appreciate you taking the time to contribute!

Will be back with more soon.

pdf · 2017-05-11T22:46:23Z

@slimsag write:

I should point out that it is hard to work on such a large PR as this

Yes, and I apologise, but with the scope of the changes I wanted to make I couldn't see much in the way of alternatives - I essentially rewrote the render chain, and not much more than the prop/attr/etc assignment code remains of the original.

I tried to include a decent level of commenting, but if there are any explanations or clarifications I can provide that will ease your efforts, please don't hesitate to ask.

Refs hexops#85

Fixes hexops#75 hexops#85

This allows the creation of elements like inline SVG. Refs hexops#54

NEW: - `Keyer` interface provides support for efficient list element re-use. Components simply need to implement the `Key` function, or the `vecty.Key` Markup may be used for HTML. BREAKING: - `Restorer` interface is now `Updater`, and its `Restore` method is now `ShouldUpdate`. The previous `Restorer` implementation was variously broken in any case, and poorly named. Additionally, it requires a `Clone` method to support passing a previous Component instance to `ShouldUpdate`. There is a limited generic version available on `vecty.Core` using reflection that may satisfy for users that do not wish to implement it themselves. - `Context` method on `Component` interface is no longer public. - `Restore` method on HTML is no longer public.

pdf · 2017-05-12T21:04:40Z

Rebased against master with the new node structure.

I'm still testing this - I think some of my DOM code may be affected by the wrappedObject.

emidoots · 2017-05-12T22:07:12Z

Thanks for the rebase. I'm still working on tests against the master branch right now. Once that's done I will be picking apart portions of your branch to review and merge separately. I'll handle rebasing your branch in this repo once I begin doing that (and will open a 2nd PR / cc you)

This allows Mount handlers that depend on sibling or parent elements to function as expected, and for performing tasks like detecting element sizes. On double-mount, perform unmount first. On double-unmount, skip event.

pdf · 2017-05-16T08:50:09Z

@slimsag FYI, I have one bug to fix here that I'm aware of, related to a scenario where Rerender is called on a Component that has already been replaced.

Batching makes updates smoother, and dedup avoids unnecessary mutations, since Component state must be maintained internally.

pdf · 2017-05-16T12:23:49Z

The above-mentioned bug should be fixed by d0d5cd0, which stops components from being re-rendered directly after they're removed, but retains the ability to pass components around and render them as children.

The latest commit (242ab03) adds batching via requestAnimationFrame, and deduplication of Component re-renders. I believe deduplication in this fashion should be safe since any component implementation I can think of must manage state internally, so we're just skipping over intermediate states. If you believe that there are some conditions where this may not be safe, we can omit dedup, but it's working well here. I may take one more pass over the code, but I wanted to get it out to obtain some feedback.

The raf dependency/polyfill I've pulled in is pretty small, so we could just add it locally if you'd prefer not to carry an external dependency for it.

@pdf

This allows the creation of elements like inline SVG. Helps #54 Author amended to @pdf instead of me (@slimsag), to provide credit for his original change at #107

emidoots · 2017-05-17T04:43:49Z

ecbef2a, "Add support for namespaced elements", is the first part of your change that I've taken a look at. I've factored it out into #110 and merged it.

emidoots · 2017-05-17T08:11:07Z

-> #112 for rebased branch

pdf · 2017-06-17T00:18:19Z

I think we may as well close this for now, let's work on getting changes in one at a time.

This was referenced May 10, 2017

Restorer is not supported by Rerender #106

Closed

Implement Mounter, trigger mount/unmount based on component equality #93

Closed

Allow component re-use #94

Closed

Add support for namespaced elements #104

Closed

pdf force-pushed the render_new branch 6 times, most recently from 55afe4b to d335c6c Compare May 10, 2017 13:28

dmitshur reviewed May 11, 2017

View reviewed changes

pdf force-pushed the render_new branch from bca7426 to 46bc303 Compare May 11, 2017 03:20

emidoots mentioned this pull request May 11, 2017

enable mocking JS calls for testing Vecty #108

Merged

pdf added 8 commits May 13, 2017 01:35

Factor out unmount behaviour

aa140ef

Refs hexops#85

Implement Mounter, trigger mount/unmount based on component equality

6d79873

Fixes hexops#75 hexops#85

Add support for namespaced elements

ecbef2a

This allows the creation of elements like inline SVG. Refs hexops#54

Move DOM manipulation to HTML, and fix incompatible elements

f014ed4

Fix for rendering with prevKeyed <-> nextKeyed transition

b8fe145

Style fixes

52b9942

Avoid unnecessary element replacement on Rerender for compatible tags

5bd864c

pdf force-pushed the render_new branch from d73d13e to 5e9ef30 Compare May 12, 2017 21:03

Defer lifecycle events until all mutations have been performed.

846b4f7

This allows Mount handlers that depend on sibling or parent elements to function as expected, and for performing tasks like detecting element sizes. On double-mount, perform unmount first. On double-unmount, skip event.

pdf added 2 commits May 13, 2017 13:16

Add jsUndefined and fix lint

eee4448

Avoid panic on HTML.node == nil

046d260

pdf force-pushed the render_new branch from 5e9ef30 to 046d260 Compare May 13, 2017 03:43

Skip Rerender for Components that have been removed

d0d5cd0

pdf force-pushed the render_new branch from 4ad9131 to 1b0d15f Compare May 16, 2017 12:20

Add deduplicating batched renderer

242ab03

Batching makes updates smoother, and dedup avoids unnecessary mutations, since Component state must be maintained internally.

pdf force-pushed the render_new branch from 1b0d15f to 242ab03 Compare May 16, 2017 12:23

emidoots mentioned this pull request May 17, 2017

proper testing of Vecty #109

Merged

emidoots mentioned this pull request May 17, 2017

Add Namespace for creating namespaced elements (e.g. for inline SVG) #110

Merged

emidoots mentioned this pull request May 17, 2017

[tracking PR] Render new #112

Closed

emidoots mentioned this pull request Jun 11, 2017

"Uncaught TypeError: Cannot read property 'style' of null" panic when Restorer interface implemented. #92

Closed

pdf closed this Jun 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render new #107

Render new #107

pdf commented May 10, 2017 •

edited

Loading

dmitshur May 11, 2017

dmitshur left a comment

dmitshur May 11, 2017

dmitshur May 11, 2017

pdf commented May 11, 2017 •

edited

Loading

emidoots commented May 11, 2017 •

edited

Loading

pdf commented May 11, 2017

pdf commented May 12, 2017 •

edited

Loading

emidoots commented May 12, 2017 •

edited

Loading

pdf commented May 16, 2017

pdf commented May 16, 2017

emidoots commented May 17, 2017 •

edited

Loading

emidoots commented May 17, 2017

pdf commented Jun 17, 2017

Render new #107

Render new #107

Conversation

pdf commented May 10, 2017 • edited Loading

dmitshur May 11, 2017

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

dmitshur May 11, 2017

Choose a reason for hiding this comment

dmitshur May 11, 2017

Choose a reason for hiding this comment

pdf commented May 11, 2017 • edited Loading

emidoots commented May 11, 2017 • edited Loading

pdf commented May 11, 2017

pdf commented May 12, 2017 • edited Loading

emidoots commented May 12, 2017 • edited Loading

pdf commented May 16, 2017

pdf commented May 16, 2017

emidoots commented May 17, 2017 • edited Loading

emidoots commented May 17, 2017

pdf commented Jun 17, 2017

pdf commented May 10, 2017 •

edited

Loading

pdf commented May 11, 2017 •

edited

Loading

emidoots commented May 11, 2017 •

edited

Loading

pdf commented May 12, 2017 •

edited

Loading

emidoots commented May 12, 2017 •

edited

Loading

emidoots commented May 17, 2017 •

edited

Loading