Skip to content
This repository was archived by the owner on Jul 30, 2018. It is now read-only.

Use native Object.assign when available. Fixes #54. #56

Closed
wants to merge 2 commits into from
Closed

Use native Object.assign when available. Fixes #54. #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1259a1
Use native `Object.assign` when available. Fixes #54.
Jul 18, 2015
1b37d75
Clean up `lang.assign`
Jul 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/has.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ add('dom-mutationobserver', function(): boolean {
add('microtasks', function () {
return has('promise') || has('host-node') || has('dom-mutationobserver');
});
add('object-assign', typeof (<any> Object).assign === 'function');
Copy link
Member

Choose a reason for hiding this comment

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

We should solidify the has naming... I know somewhere else we discussed that we would prefer to use es6-object-assign for something like this (and object-observe should be [in theory] es7-object-observe and promise should be es6-promise).

Copy link
Author

Choose a reason for hiding this comment

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

I vaguely remember that discussion, but also vaguely remember no definitive consensus, so I decided to follow the convention already in place.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it wasn't specifically directed at you, but more the wider bit reminding that we didn't close it off... I guess we can go back later and clean that up.

Copy link
Member

Choose a reason for hiding this comment

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

I have opened an issue in dojo/meta#5.

add('object-observe', typeof (<any> Object).observe === 'function');
add('postmessage', typeof postMessage === 'function');
add('promise', typeof global.Promise !== 'undefined');
Expand Down
22 changes: 14 additions & 8 deletions src/lang.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ interface MixinArgs {
target: {};
}

interface ObjectAssignConstructor extends ObjectConstructor {
assign(target: {}, ...sources: {}[]): {}
}

function _mixin(kwArgs: MixinArgs): {} {
const deep = kwArgs.deep;
const inherited = kwArgs.inherited;
Expand Down Expand Up @@ -73,14 +77,16 @@ function _mixin(kwArgs: MixinArgs): {} {
* @param sources Any number of objects whose enumerable own properties will be copied to the target object
* @return The modified target object
*/
export function assign(target: {}, ...sources: {}[]): {} {
return _mixin({
deep: false,
inherited: false,
sources: sources,
target: target
});
}
export const assign = has('object-assign') ?
(<ObjectAssignConstructor> Object).assign :
function (target: {}, ...sources: {}[]): {} {
return _mixin({
deep: false,
Copy link
Member

Choose a reason for hiding this comment

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

Just realised you are checking the has flag in line 76 and then executing code that should never be accessible because this branch shouldn't be followed if has('object-assign') is true.

Copy link
Member

Choose a reason for hiding this comment

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

Following that logic, wouldn't your ObjectAssignConstructor interface then be needed both in the has test and here?

I'm usually pro-avoiding-any, but it seems like a whole lot of pointless runaround for something that ultimately doesn't matter in this case (at least as far as type information for the external API is concerned).

Edit: wait, I guess it does matter, if we're assigning Object.assign directly in that case. Darnit.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we have to have it where we assign Object.assign to the function to preserve the signature... Not wholly necessary in the has check though.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fair.

Copy link
Author

Choose a reason for hiding this comment

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

Excellent! Another mistake to keep me humble! Regarding the extended type, that proposal does seem like the best way to ensure the interface is preserved.

inherited: false,
sources: sources,
target: target
});
}

/**
* Creates a new object from the given prototype, and copies all enumerable own properties of one or more
Expand Down