Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite render() to use NodePart #491

Merged
merged 2 commits into from
Sep 12, 2018
Merged

Rewrite render() to use NodePart #491

merged 2 commits into from
Sep 12, 2018

Conversation

justinfagnani
Copy link
Collaborator

This supersedes #445

Fixes #422

@ruphin
Copy link
Contributor

ruphin commented Sep 12, 2018

I was looking into this exact patch yesterday. I came up with an implementation very similar to this, although yours looks a bit cleaner. The biggest problem with this change is that each render(<something>) now injects two markers that it didn't need before, which affects performance.

I think it should be possible to use a single marker. A NodePart only needs one node to exist (before or after) so it can reference it's parent. If the other node doesn't exist, it can assume that it is the first node (or the last node) in the context. This will require some refactoring of parts in general, but I feel like the number of required markers can be cut down, which will not only affect render(), but other instances of parts as well. My own implementation of NodePart does exactly this, and although it adds a bit of complexity, it creates one less marker for most instances of NodePart.

@justinfagnani
Copy link
Collaborator Author

@ruphin there are actually a number of places where we can reduce markers. In this case (and a few others) we should be able to get it down to 0 markers. I think I want to get this in and attack markers more holistically.

@justinfagnani justinfagnani merged commit 59f2788 into master Sep 12, 2018
@justinfagnani justinfagnani deleted the node-render branch September 12, 2018 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants