Skip to content
This repository has been archived by the owner on Apr 4, 2019. It is now read-only.

[PERF] Don't Attempt To Sanitize Unsanitizeable #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chadhietala
Copy link
Collaborator

This adds an identity check for falsy values before the value is
sanitized. Currently we do work to sanitize things like undefined.

This is from a bench of a thousand link-tos
Before:
screen shot 2015-10-07 at 11 56 21 am

After:
screen shot 2015-10-07 at 11 56 45 am

@stefanpenner
Copy link
Collaborator

such Unsanitiveable

@chadhietala chadhietala changed the title [PERF] Don't Attempt To Sanitive Unsanitiveable [PERF] Don't Attempt To Sanitive Unsanitizeable Oct 7, 2015
@@ -110,7 +110,7 @@ AttrMorph.prototype.setContent = function (value) {
if (this.lastValue === value) { return; }
this.lastValue = value;

if (this.escaped) {
if (this.escaped && this.valueIdentity(value)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this just just mean all truthy things are left unescaped ?

On another unrelated note, the this.escaped flag name sucks, we should have named it needsEscape or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand what is going on here but if this.escape === true, then the value we are trying to escape should be at least truthy. Currently we run the block with undefined or strings. The sanitized value of undefined will be undefined, but clearly there is a micro-opt to be had here. I added the function instead of doing

if (this.escaped && value) {

}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this aims to skips escaping if the value is falsy. As we believe no falsey values will need escaping.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the reason for the method?

@stefanpenner
Copy link
Collaborator

needs señor tests.

This adds an identity check for falsy values before the value is
sanitized. Currently we do work to sanitize things like `undefined`.
@chadhietala chadhietala changed the title [PERF] Don't Attempt To Sanitive Unsanitizeable [PERF] Don't Attempt To Sanitize Unsanitizeable Oct 9, 2015
@chadhietala
Copy link
Collaborator Author

Not really sure how to test this as the sanitization function is local. Not sure how helpful the test is.

@@ -17,6 +17,20 @@ test("can update a dom node", function(){
equal(element.getAttribute('id'), 'twang', 'id attribute is set');
});

test("setting content to undefined calls _update with undefiend", function(){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "undefiend" should be "undefined"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants