Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Standards compliant approach to allowing _.extend to copy ES5 Getters/Setters #986

Closed
wants to merge 2 commits into from

4 participants

@kevinlacotaco

Looked through the approaches in the past and saw they were not standards compliant.

Uses Object.defineProperty() and Object.getOwnPropertyDescriptor() which are at least partially supported back to IE8.

@michaelficarra
Collaborator

Partial support on IE8 is deceptive. It supports them for DOM nodes only, which is basically useless.

@kevinlacotaco

Is there a list of supported browsers anywhere I should be looking at?

I could always put a check in to see if those methods are supported and then use that otherwise use the old approach.

@michaelficarra
Collaborator

This would also introduce an inconsistency in the behaviour of _.extend in different browsers, which is exactly the opposite of underscore's goal of implementing a functional library that can be used in a browser-agnostic way.

@kevinlacotaco

The checks would add an inconsistency?

What browsers are supported?

It delegates to built-in functions, if present, so modern browsers will use the native implementations of forEach, map, reduce, filter, every, some and indexOf.

I don't really see how this is any different then delegating to the built in functions that underscore is already using.

I have pushed a version with checks.

@michaelficarra
Collaborator

I think I know where the disconnect is. You probably think getters/setters either exist or don't. In some JS implementations, getters/setters exist, but there's no support for defining/describing them. So when encountering an object with getters/setters in one of these interpreters, you will have observable differences from the behaviour of the same object in an engine that supports defining/describing these properties. Does that make sense?

@jashkenas jashkenas closed this
@dalgard

This is still relevant, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 2 deletions.
  1. +13 −0 test/objects.js
  2. +10 −2 underscore.js
View
13 test/objects.js
@@ -60,6 +60,19 @@ $(document).ready(function() {
} catch(ex) {}
equal(result.a, 1, 'should not error on `null` or `undefined` sources');
+
+ var value = 'Test value';
+ result = _.extend({}, {
+ get test() {
+ return this._test;
+ },
+ set test (newValue) {
+ equal(newValue, value, 'ES5 Setter was called with the correct arguments');
+ this._test = newValue;
+ }
+ });
+ result.test = value;
+ equal(result.test, value, 'ES5 Getter was copied over successfully');
});
test("pick", function() {
View
12 underscore.js
@@ -41,7 +41,9 @@
nativeLastIndexOf = ArrayProto.lastIndexOf,
nativeIsArray = Array.isArray,
nativeKeys = Object.keys,
- nativeBind = FuncProto.bind;
+ nativeBind = FuncProto.bind,
+ nativeDefine = Object.defineProperty,
+ nativeDescriptor = Object.getOwnPropertyDescriptor;
// Create a safe reference to the Underscore object for use below.
var _ = function(obj) {
@@ -777,9 +779,15 @@
// Extend a given object with all the properties in passed-in object(s).
_.extend = function(obj) {
each(slice.call(arguments, 1), function(source) {
+ var supported = ((nativeDefine && Object.defineProperty === nativeDefine) &&
+ (nativeDescriptor && Object.getOwnPropertyDescriptor === nativeDescriptor));
if (source) {
for (var prop in source) {
- obj[prop] = source[prop];
+ if (supported) {
+ nativeDefine(obj, prop, nativeDescriptor(source, prop));
+ } else {
+ obj[prop] = source[prop];
+ }
}
}
});
Something went wrong with that request. Please try again.