Avoid Object.prototype collisions with defaults #3843

jridgewell · 2015-10-30T00:36:03Z

This is definitely an underscore concern, but it's easier to demo in Backbone code.

akre54 · 2015-10-30T15:18:11Z

test/model.js

    });
-    model = new Defaulted({two: undefined});


Why'd you get rid of this test case?

It's testing the same thing as the one above it, no?

No... it's testing that passing undefined is thrown away, not that the key isn't passed at all.

No... it's testing that passing undefined is thrown away, not that the key isn't passed at all.

Isn't that this test?

var defaults = {foo: true};

With my "defaults" implementation:

defaults = {foo: true}; defaults({}, defaults, {foo: undefined}); // { foo: true } defaults({}, defaults, {bar: undefined}); // { foo: true, bar: undefined }

The only difference with mine is it won't respect properties that are already on the dest object:

_.defaults({foo: false}, {foo: true}); // { foo: false } defaults({foo: false}, {foo: true}); // { foo: true }

Isn't that this test?

Sorry, missed that in the diff.

The only difference with mine is it won't respect properties that are already on the dest object

Then I'm confused. That changes _.default's behavior completely, no?

We're only using defaults with an "empty" destination object, so it works the same. Externally, this wouldn't be acceptable.

Yeah but I dunno. Either we should fix this in Underscore or we shouldn't fix this at all. What's good for the goose is good for the gander...

If we made it return a new instance instead of modifying the destination, it would work perfectly. We're kind of headed down that path with jashkenas/underscore#2232.

jdalton · 2015-12-15T16:40:08Z

For reference here's the current fix in lodash lodash/lodash@8a3842b.
It preserves the existing defaults iteration order and behavior of not overwriting non-undefined values.

I think it's fine to have this in Backbone temporarily until it can be addressed in a released version of Underscore.

akre54 · 2015-12-15T16:52:58Z

I dunno. I feel like this is something that should be fixed in Underscore, if at all. Who names their keys hasOwnProperty? Attribute keys and collection ids really shouldn't be arbitrary like this.

jdalton · 2015-12-15T17:34:41Z

I dunno. I feel like this is something that should be fixed in Underscore, if at all. Who names their keys hasOwnProperty?

It's less those classic Object.prototype method names and more of the new ones like watch in Firefox. I'm cool with punting on it for now as as wontfix too.

akre54 · 2015-12-15T17:57:46Z

To be sure, watch is a pretty dumb decision on FF's part, but it's not insurmountable. Unless there's a really good reason I'd rather not introduce a subtle backwards compatibility bug like this.

jdalton · 2015-12-15T18:09:36Z

Yep, though I think it can be tackled without back compat pains. The likely issues I see are with watch, unwatch, constructor, toString, and toSource properties. However, since this hasn't been tackled yet, several years in, it makes sense for the fix to be designated as a lower priority (not critical for 1.2.4).

jridgewell · 2016-01-31T21:43:08Z

Repinging this: I've updated the PR to use _.extends and _.defaults to work around the conflicts. What does everyone else think?

jridgewell · 2016-02-03T16:11:11Z

Ping @akre54 since you're reviewing things today. 😉

akre54 · 2016-02-03T16:16:30Z

Hows the perf hit when there are a few keys?

jridgewell · 2016-02-03T18:03:44Z

http://jsperf.com/defaults-extend/2

jridgewell · 2016-02-04T17:19:47Z

Ping. I want to get this one in.

akre54 · 2016-02-04T17:23:36Z

For the record, I think this (_.defaults(_.extend({}, defaults, attrs), defaults)) is a really ugly hack for a minor to nonexistent problem, but the perf hit isn't terrible. Hope this is the last of it.

Avoid Object.prototype collisions with defaults

akre54 · 2016-02-04T17:24:51Z

test/model.js

@@ -989,8 +994,8 @@

  QUnit.test('`previous` for falsey keys', function(assert) {
    assert.expect(2);
-    var model = new Backbone.Model({0: true, '': true});
-    model.set({0: false, '': false}, {silent: true});
+    var model = new Backbone.Model({'0': true, '': true});


Whys this needed? JS automatically casts keys to strings

It was a liter warning for inconsistent string usage. Must have committed it by accident. 😊

Linting rules are not perfect 😞

All good, they're much better now than before.

This first commit here makes the changes made in pull jashkenas#3843 unnecessary and we can revert those changes. Since it will be using prototypeless objects we don't have to worry about the collisions

akre54 reviewed Oct 30, 2015
View reviewed changes

jridgewell force-pushed the defaults-prototype-collision branch 2 times, most recently from b8bd90f to aa421d9 Compare October 30, 2015 15:54

This was referenced Nov 2, 2015

Make defaults an immutable copy jashkenas/underscore#2333

Closed

v1.2.4 #3839

Closed

jridgewell force-pushed the defaults-prototype-collision branch from aa421d9 to 325fe1c Compare January 31, 2016 21:42

jridgewell force-pushed the defaults-prototype-collision branch from 325fe1c to 76077f5 Compare January 31, 2016 21:47

jridgewell changed the title ~~Use modified defaults algorithm to avoid Object.prototype collisions~~ Avoid Object.prototype collisions with defaults Feb 3, 2016

Use _.extend to avoid Object.prototype collisions

92fb299

jridgewell force-pushed the defaults-prototype-collision branch from 76077f5 to 92fb299 Compare February 3, 2016 17:56

akre54 added the change label Feb 4, 2016

akre54 added a commit that referenced this pull request Feb 4, 2016

Merge pull request #3843 from jridgewell/defaults-prototype-collision

08a486c

Avoid Object.prototype collisions with defaults

akre54 merged commit 08a486c into jashkenas:master Feb 4, 2016

akre54 reviewed Feb 4, 2016
View reviewed changes

jashkenas added the fixed label Sep 7, 2016

jgonggrijp mentioned this pull request Jul 19, 2023

Clean up misguided legacy changes #4266

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid Object.prototype collisions with defaults #3843

Avoid Object.prototype collisions with defaults #3843

jridgewell commented Oct 30, 2015

akre54 Oct 30, 2015

jridgewell Oct 30, 2015

akre54 Oct 30, 2015

akre54 Oct 30, 2015

jridgewell Oct 30, 2015

akre54 Oct 30, 2015

jridgewell Oct 30, 2015

akre54 Oct 30, 2015

jridgewell Oct 30, 2015

jdalton commented Dec 15, 2015

akre54 commented Dec 15, 2015

jdalton commented Dec 15, 2015

akre54 commented Dec 15, 2015

jdalton commented Dec 15, 2015

jridgewell commented Jan 31, 2016

jridgewell commented Feb 3, 2016

akre54 commented Feb 3, 2016

jridgewell commented Feb 3, 2016

jridgewell commented Feb 4, 2016

akre54 commented Feb 4, 2016

akre54 Feb 4, 2016

jridgewell Feb 4, 2016

captbaritone Feb 4, 2016

jridgewell Feb 4, 2016

Avoid Object.prototype collisions with defaults #3843

Avoid Object.prototype collisions with defaults #3843

Conversation

jridgewell commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton commented Dec 15, 2015

akre54 commented Dec 15, 2015

jdalton commented Dec 15, 2015

akre54 commented Dec 15, 2015

jdalton commented Dec 15, 2015

jridgewell commented Jan 31, 2016

jridgewell commented Feb 3, 2016

akre54 commented Feb 3, 2016

jridgewell commented Feb 3, 2016

jridgewell commented Feb 4, 2016

akre54 commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment