implement equals method for equivalent ContextClosure objects #20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… source template The desire of ignoreTemplate is to prevent over-computing bindings for templates. However, wrapping templates in ContextClosures was preventing us from detecting the template that should be ignored. This resolves the issue by ignoring the base template of the ContextClosure
… and not wrapped already It could be possible for an object to have an unwrapped Template in it that is returned by looking up segemtns, so this shouldn't be an else. Also, wrapping a template in two ContextClosures isn't needed, since the inner ContextClosure is going to be the only one that has an effect. Thus, do check for an existing ContextClosure to avoid direct double wrapping.
|
BTW, I edited the PR description to refer to the Saddle PR - I like cross-linking. I also added some description for the new |
ericyhwang
approved these changes
Aug 8, 2018
ericyhwang
reviewed
Aug 8, 2018
lib/templates.js
Outdated
| var closureContext = context.closureChild(this.context); | ||
| return this.template.dependencies(closureContext, options); | ||
| }; | ||
| ContextClosure.prototype.equals = function(other) { | ||
| return (other instanceof ContextClosure) && | ||
| (other.template === this.template) && |
There was a problem hiding this comment.
Do we need to worry about this.template or other.template themselves being ContextClosures?
There was a problem hiding this comment.
Good idea. Let's recursively use the equals function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolve issue with determining if the same template is being rendered and binding should not update (because the template has not changed). This issue was created because the template was wrapped in a new ContextClosure object.
Solution is to override the base
Template#equalsmethod (added in derbyjs/saddle#27) withContextClosure#equals, which treats ContextClosures as equal if they have the same template and context properties - meaning they would render identical output in all cases. This fixes issues with over-rendering.This also has
DependencyOptionsfully unwrap itsignoreTemplatefield if the field is a ContextClosure, asDepedencyOptions.shouldIgnoreTemplate(template, options)will always get atemplategenerated by derby-parsing, which means it will never get a ContextClosure.