Skip to content

.escape should return null instead of "" if the attribute is null.#1227

Closed
markbates wants to merge 1 commit into
jashkenas:masterfrom
markbates:null_escape_bug
Closed

.escape should return null instead of "" if the attribute is null.#1227
markbates wants to merge 1 commit into
jashkenas:masterfrom
markbates:null_escape_bug

Conversation

@markbates

Copy link
Copy Markdown

The .escape function on Backbone.Model is returning an empty string if the attribute value is null. It should return null to match the original value. This can cause issues where you're checking the existence of an attribute before acting on it.

@edwardmsmith

Copy link
Copy Markdown

Hmm, I can see both sides of this one.

If escape is intended for printing values to html, I can see the value in converting null to an empty string. If you're fetching a value for testing, it may be best to use .get() for the test value.

On the other hand, its inconsistent with underscore.

This change could have significant impact on existing apps.

@markbates

Copy link
Copy Markdown
Author

Yeah, there are definitely two sides to this one. In regards to printing values, if you try to print a JavaScript null to HTML it prints an empty string, so that's not going to be an issue with existing apps. The place where it could be an issue with existing apps is if you're concatenating a null and a string. That would result in something like "nullfoo".

@braddunbar

Copy link
Copy Markdown
Collaborator

This goes right along with jashkenas/underscore#556 and jashkenas/underscore#559. Whatever the decision is, I think these issues should be handled consistently.

@jashkenas

Copy link
Copy Markdown
Owner

Why would you check the existence of an attribute after escaping it? The escape function is intended to work on strings... and must return a string. We shouldn't change this.

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.

4 participants