Add _.create method #1907

jamiebuilds · 2014-10-23T22:42:10Z

_.create is useful as both a "polyfill" for Object.create and a utility for simple prototype inheritance.

Implementation is adapted from Lodash's _.create method. I've kept the baseCreate function for the usage in the methods @jdalton mentioned here.

function Person(name, occupation) {
  this.name = name;
  this.occupation = occupation;
}

function Developer(name) {
  Person.call(this, name, 'developer');
}

Developer.prototype = _.create(Person.prototype, {
  constructor: Developer
});

var developer = new Developer('Jeremy Ashkenas');
developer instanceof Developer;
// >> true

developer instanceof Person;
// >> true

Ref: @jdalton #1901 (comment)

jdalton · 2014-10-23T23:30:43Z

Might make executeBound take advantage of this in this PR too.

jdalton · 2014-10-23T23:42:28Z

underscore.js

@@ -90,6 +91,23 @@
    return cb(value, context);
  };

+  // An internal function for creating a new object that inherts from another.
+  var baseCreate = nativeCreate || (function() {
+    function Object() {}


Using function Object() {} will cause an issue in an old browser (not sure off the top of my head which) with the use of the object literal on line 107. The literal will incorrectly use the local Object constructor instead. I'll see if I can dig up more detail on that.

Use Ctor?

Lodash has the same thing in baseCreate, are you just dropping support for that browser or are you doing something magical?

Use Ctor?

I use context.Object() which is gross but I really like the way the object looks in the dev console when the constructor is Object 😸

@jdalton Hmm, I do like the Object {} I think it'd be better to use Ctor for this.

are you just dropping support for that browser or are you doing something magical?

Ours is:

function baseCreate(prototype) { return isObject(prototype) ? nativeCreate(prototype) : {}; }

@jdalton I was referring to the Object in:

if (!nativeCreate) { baseCreate = (function() { function Object() {} // << return function(prototype) { if (isObject(prototype)) { Object.prototype = prototype; var result = new Object; Object.prototype = null; } return result || context.Object(); }; }()); }

@jdalton Hmm, I do like the Object {} I think it'd be better to use Ctor for this.

I'm not too concerned about that, this will only be for environments without Object.create

result || context.Object();

That references global.Object

Yap, I avoided issues by doing context.Object() as mentioned above.

Oooooooh, I misread what you said.

A wild understanding appears

jamiebuilds · 2014-10-24T00:20:06Z

@jdalton Updated executeBound.

megawac · 2014-10-24T00:34:27Z

underscore.js

+  // created object.
+  _.create = function(prototype, props) {
+    var result = baseCreate(prototype);
+    return props ? _.extend(result, props) : result;


I haven't read the spec for Object.create, is it own props or in props used

own props: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.3.5

Alright well we may have to defer to 2.0 in that case

Could I update it to use _.keys until then?

@thejameskyle I haven't opened an issue yet, but I would like to maintain _.extend behaviour to use in props and add a _.assign function for own properties.

/cc @akre54 @jashkenas @jdalton

Ya know my stance on it.

@jdalton and for those who don't? 👉😁👈

and for those who don't? 👉😁👈

I think _.assign (aliased as _.extend) is enough and other cases can fall to _.create. It's worked well in my experience. That said I also offer things like _.keysIn, _.forIn, and _.valuesIn so devs can easily create their own mixins.

@megawac I've updated _.create to use _.keys for now so it isn't blocked.

Yep, I'm definitely for adding keysIn, valuesIn and forIn

megawac · 2014-10-26T00:24:37Z

underscore.js

+  // Creates an object that inherits from the given prototype object.
+  // If additional properties are provided then they will be added to the
+  // created object.
+  _.create = function(prototype, props) {


I think this belongs in the Objects part of the codebase

It's not already? The closest header above says "Object Functions"

My bad got lost in the source

On Sat, Oct 25, 2014 at 9:08 PM, James Kyle [email protected]
wrote:

In underscore.js:

@@ -998,6 +1012,23 @@
return obj;
};

// Creates an object that inherits from the given prototype object.

// If additional properties are provided then they will be added to the

// created object.

_.create = function(prototype, props) {

It's not already? The closest header above says "Object Functions"

—
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/underscore/pull/1907/files#r19378463.

michaelficarra · 2014-10-26T01:41:11Z

This looks ready to go. Can I get a 👍?

megawac · 2014-10-26T01:45:27Z

Yeah, I think we should merge #1910 > #1908 > #1907 > #1901 as they are modularally dependent.

megawac · 2014-11-21T16:39:14Z

underscore.js

+      for (i = 0; i < length; i++) {
+        key = keys[i];
+        result[key] = props[key];
+      }


mind updating to use _.assign?

jamiebuilds · 2014-11-22T18:53:30Z

@megawac Updated.

megawac · 2014-11-23T03:31:16Z

underscore.js

+  // An internal function for creating a new object that inherts from another.
+  var baseCreate = nativeCreate ? function(prototype) {
+    return _.isObject(prototype) ? nativeCreate(prototype) : {};
+  } : function(prototype) {


Tests pass with

nativeCreate || function(prototype) { ...

Are better tests neaded or is that correct

Ah okay, Object.create() or Object.create(1) for instance will error. In that case should the default value be {} or Object.create(null)?

Returning {} make more sense, since it'll mirror the non-nativeCreate path where we can't create a prototype-less object.

For some reason, I didn't think of setting prototype to null in the non-nativeCreate path. Ignore my previous comment.

jamiebuilds · 2014-11-24T05:27:56Z

Anything else needed here?

megawac · 2014-11-24T15:37:53Z

@thejameskyle mind just adding a test to cover this case https://github.com/jashkenas/underscore/pull/1907/files#discussion_r20761788?

Also thoughts on return nativeCreate(_.isObject(prototype) ? prototype : null) vs what you're doing now?

jamiebuilds · 2014-11-29T20:16:57Z

Updated with more tests and returning nativeCreate(null) instead of {}. @jdalton this will be a small difference in behavior from lodash.

@megawac Squashed so it should be good now.

jamiebuilds · 2014-11-29T20:17:43Z

~~Gah whoops, hold off I messed up with Object.create(null).~~ Fixed

jamiebuilds · 2014-11-29T21:04:58Z

@jridgewell [ref] updated thanks.

jridgewell · 2014-11-29T21:13:09Z

Wait a second, I knew there was a reason I thought we couldn't create a prototype-less object without nativeCreate.

function Test() {}
Test.prototype = null;
var t = new Test();
t.__proto__ === Object.prototype; // => true
t.toString(); // => "[object Object]"

Ignore my comment telling you to ignore my comment. It would be better to take @jdalton's path here and instead return a new object literal, since we can't mirror functionality in the non-nativeCreate path.

jamiebuilds · 2014-11-29T21:18:19Z

@jridgewell After speaking to @megawac I decided to inline the nativeCreate. jsperf

jridgewell · 2014-11-29T21:39:22Z

Both inline and separated are fine with me. But I think 70a120d-L117 should be prototype = _.isObject(prototype) ? prototype : {};, since it would return consistent results for browsers with and without nativeCreate.

jamiebuilds · 2014-11-29T21:42:48Z

@jridgewell I was thinking about making that just if (!_.isObject(prototype) return {};

jridgewell · 2014-11-29T21:53:48Z

That would be fine too. 😄

megawac · 2014-11-30T01:27:17Z

Alright LGTM now

/cc @michaelficarra

Add _.create method

jdalton reviewed Oct 23, 2014
View reviewed changes

megawac reviewed Oct 24, 2014
View reviewed changes

This was referenced Oct 24, 2014

Add _.assign function #1908

Merged

Add _.keysIn method #1910

Merged

megawac reviewed Oct 26, 2014
View reviewed changes

megawac added the enhancement label Nov 3, 2014

megawac mentioned this pull request Nov 14, 2014

add _.newer method and tests #1932 #1937

Closed

megawac reviewed Nov 21, 2014
View reviewed changes

underscore.js

for (i = 0; i < length; i++) {

key = keys[i];

result[key] = props[key];

}

Copy link

Collaborator

megawac Nov 21, 2014

Choose a reason for hiding this comment

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

mind updating to use _.assign?

jamiebuilds force-pushed the create branch from 00c032f to a708ab5 Compare November 22, 2014 18:50

megawac reviewed Nov 23, 2014
View reviewed changes

jamiebuilds force-pushed the create branch from fd9d5c9 to fc03e4b Compare November 29, 2014 20:13

jamiebuilds force-pushed the create branch 2 times, most recently from 440a3c0 to 7de85c4 Compare November 29, 2014 21:03

jamiebuilds force-pushed the create branch from 7de85c4 to 70a120d Compare November 29, 2014 21:16

Add _.create method

deb1572

jamiebuilds force-pushed the create branch from 70a120d to deb1572 Compare November 29, 2014 21:57

jashkenas added a commit that referenced this pull request Dec 1, 2014

Merge pull request #1907 from thejameskyle/create

7cfd0f9

Add _.create method

jashkenas merged commit 7cfd0f9 into jashkenas:master Dec 1, 2014

jashkenas added the fixed label Dec 1, 2014

jamiebuilds deleted the create branch December 1, 2014 17:40

megawac mentioned this pull request Jan 19, 2015

Remove Behaviors in favor of Mixins marionettejs/backbone.marionette#2216

Closed

jridgewell mentioned this pull request Feb 20, 2015

Add _.create method #2074

Merged

Add _.create method #1907

Add _.create method #1907

Conversation

jamiebuilds commented Oct 23, 2014

jdalton commented Oct 23, 2014

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

Choose a reason for hiding this comment

jamiebuilds commented Oct 24, 2014

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

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

michaelficarra commented Oct 26, 2014

megawac commented Oct 26, 2014

Choose a reason for hiding this comment

jamiebuilds commented Nov 22, 2014

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

jamiebuilds commented Nov 24, 2014

megawac commented Nov 24, 2014

jamiebuilds commented Nov 29, 2014

jamiebuilds commented Nov 29, 2014

jamiebuilds commented Nov 29, 2014

jridgewell commented Nov 29, 2014

jamiebuilds commented Nov 29, 2014

jridgewell commented Nov 29, 2014

jamiebuilds commented Nov 29, 2014

jridgewell commented Nov 29, 2014

megawac commented Nov 30, 2014