Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1053 Introduce CKEDITOR.tools.object.merge #1055

Merged
merged 10 commits into from
Oct 17, 2017
53 changes: 53 additions & 0 deletions core/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -1991,6 +1991,59 @@
}

return null;
},

/**
* Merges two objects and returns new one.
Copy link
Contributor

Choose a reason for hiding this comment

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

An example would on how it actually merges would be a brilliant idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good if example would also illustrate what happens if both objects have properties with the same name/key.

*
* var obj1 = {
* a: 1,
* conflicted: 10,
* obj: {
* c: 1
* }
* },
* obj2 = {
* b: 2,
* conflicted: 20,
* obj: {
* d: 2
* }
* };
*
* CKEDITOR.tools.object.merge( obj1, obj2 );
*
* This code produces the following object;
*
* {
* a: 1,
* b: 2,
* conflicted: 20,
* obj: {
* c: 1,
* d: 2
* }
* }
*
* @param {Object} obj1 Source object, which will be used to create a new base object.
* @param {Object} obj2 An object, which properties will be merged to the base one.
Copy link
Contributor

Choose a reason for hiding this comment

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

An object, which properties will be merged to the base one.

I think this is a little misleading, as properties are not merged to the base object, but to its copy, so the base object is not modified. And it is also inconsistent with method description which suggest the new object is created (Merges two objects and returns new one).

ATM I don't have any clear idea how it could be described. I checked in other libs/docs how it is described there:

but still not sure. Maybe you could come up with something reasonable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe something like that?

obj1 Source object, which will be used to create a new base object.

And then obj2 description is already good :D WDYT @f1ames?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Comandeer works for me 👍

* @returns {Object} Merged object.
* @member CKEDITOR.tools.object
*/
merge: function( obj1, obj2 ) {
var tools = CKEDITOR.tools,
copy1 = tools.clone( obj1 ),
copy2 = tools.clone( obj2 );

tools.array.forEach( tools.objectKeys( copy2 ), function( key ) {
if ( typeof copy2[ key ] === 'object' && typeof copy1[ key ] === 'object' ) {
copy1[ key ] = tools.object.merge( copy1[ key ], copy2[ key ] );
} else {
copy1[ key ] = copy2[ key ];
}
} );

return copy1;
}
}
};
Expand Down
34 changes: 34 additions & 0 deletions tests/_benderjs/ckeditor/static/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,40 @@
assert.areSame( expected, bender.tools.compatHtml( actual, false, true, false, true, true ), message );
};

/**
* Asserts that two objects are deep equal.
*
* @param {Object} expected
* @param {Object} actual
* @param {String} [message]
*/
bender.objectAssert.areDeepEqual = function( expected, actual, message ) {
// Based on http://yuilibrary.com/yui/docs/api/files/test_js_ObjectAssert.js.html#l12.
var expectedKeys = YUITest.Object.keys( expected ),
actualKeys = YUITest.Object.keys( actual );

YUITest.Assert._increment();

// First check keys array length.
if ( expectedKeys.length != actualKeys.length ) {
YUITest.Assert.fail( YUITest.Assert._formatMessage( message,
'Object should have ' + expectedKeys.length + ' keys but has ' + actualKeys.length ) );
}

// Then check values.
for ( var name in expected ) {
if ( expected.hasOwnProperty( name ) ) {
if ( expected[ name ] && typeof expected[ name ] === 'object' ) {
bender.objectAssert.areDeepEqual( expected[ name ], actual[ name ] );
}
else if ( expected[ name ] !== actual[ name ] ) {
throw new YUITest.ComparisonFailure( YUITest.Assert._formatMessage( message,
'Values should be equal for property ' + name ), expected[ name ], actual[ name ] );
}
}
}
};

// Add support test ignore.
YUITest.Ignore = function() {};

Expand Down
62 changes: 62 additions & 0 deletions tests/core/tools/object.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,68 @@

returned = this.object.findKey( inputObject, innerObject );
assert.areSame( returned, 'c' );
},

// (#1053)
'test object.merge': function() {
var obj1 = {
one: 1,
conflicted: 10,
falsy: false,
nully: null,
obj: {
nested1: 1,
nestedObj: {
a: 3
}
},
array: [ 1, 2 ]
},
obj2 = {
two: 2,
conflicted: 20,
truthy: true,
undef: undefined,
obj: {
nested2: 2,
nestedObj: {
b: 4
}
},
array: [ 3, 4 ]
},
expected = {
one: 1,
two: 2,
conflicted: 20,
falsy: false,
truthy: true,
nully: null,
undef: undefined,
obj: {
nested1: 1,
nested2: 2,
nestedObj: {
a: 3,
b: 4
}
},
array: [ 3, 4 ]
},
actual = this.object.merge( obj1, obj2 );

assert.areNotSame( obj1, actual, 'Merging does not modify obj1' );
assert.areNotSame( obj2, actual, 'Merging does not modify obj2' );
objectAssert.areDeepEqual( expected, actual, 'Merging produces correct object' );

// Reversed merge should produce same object, but with different conflicted and array properties.
expected.conflicted = 10;
expected.array = [ 1, 2 ];
actual = this.object.merge( obj2, obj1 );

assert.areNotSame( obj1, actual, 'Merging does not modify obj1' );
assert.areNotSame( obj2, actual, 'Merging does not modify obj2' );
objectAssert.areDeepEqual( expected, actual, 'Merging produces correct object' );
}
} );
} )();
28 changes: 0 additions & 28 deletions tests/plugins/copyformatting/_helpers/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,6 @@

'use strict';

// Based on http://yuilibrary.com/yui/docs/api/files/test_js_ObjectAssert.js.html#l12.
YUITest.ObjectAssert.areDeepEqual = function( expected, actual, message ) {
var expectedKeys = YUITest.Object.keys( expected ),
actualKeys = YUITest.Object.keys( actual ),
areEqual = YUITest.ObjectAssert.areEqual;

YUITest.Assert._increment();

// First check keys array length.
if ( expectedKeys.length != actualKeys.length ) {
YUITest.Assert.fail( YUITest.Assert._formatMessage( message,
'Object should have ' + expectedKeys.length + ' keys but has ' + actualKeys.length ) );
}

// Then check values.
for ( var name in expected ) {
if ( expected.hasOwnProperty( name ) ) {
if ( typeof expected[ name ] === 'object' ) {
areEqual( expected[ name ], actual[ name ] );
}
else if ( expected[ name ] !== actual[ name ] ) {
throw new YUITest.ComparisonFailure( YUITest.Assert._formatMessage( message,
'Values should be equal for property ' + name ), expected[ name ], actual[ name ] );
}
}
}
};

// Safari and IE8 use text selection and all other browsers use element selection. Therefore we must normalize it.
function fixHtml( html ) {
if ( ( CKEDITOR.env.webkit && !CKEDITOR.env.chrome ) || ( CKEDITOR.env.ie && CKEDITOR.env.version === 8 ) ) {
Expand Down