CKEDITOR.tools.object helper methods improvements

[jacekbogdanski]

Type of report

Feature request

Provide description of the new feature

#3120 issue is caused by the ECMA3 DontEnum attribute. The attribute is set on some object properties which cannot be enumerated, so CKEDITOR.tools.extend method is unable to correctly copy object prototype properties.

I would like to take advantage of the situation and improve our CKEDITOR.tools.object helper methods during bug fixing:

• CKEDITOR.tools.objectKeys - should include DontEnum properties to fix the issue for IE8
• CKEDITOR.tools.object.forEach - fixing the issue with for in loop for IE8
• CKEDITOR.tools.object.reduce - the same as above
• CKEDITOR.tools.extend - fixed by using CKEDITOR.tools.object.forEach function instead of for in loop

Both forEach and reduce function will have pre-implemented DontEnum bug fix (by using CKEDITOR.tools.objectKeys which will reduce the possibility of the issue in the future.

[Comandeer]

TBH I don't like proposed changes. Personally I'd implement functions equivalent to native functions that operate on objects:

• Object.keys → CKEDITOR.tools.object.keys (basically alias to CKEDITOR.tools.objectKeys)
• Object.values → CKEDITOR.tools.values
• Object.entries → CKEDITOR.tools.entries

Having these low-level functions we can implement CKEDITOR.tools.object.forEach and CKEDITOR.tools.object.reduce simply as operations on arrays returned by mentioned functions.

[f1ames]

Have to agree with @Comandeer that functions reflecting native ones will be more intuitive and easier to use (as you already know the API), so it's 👍 from me.

One clarification, though:

Having these low-level functions we can implement CKEDITOR.tools.object.forEach and CKEDITOR.tools.object.reduce simply as operations on arrays returned by mentioned functions.

Do you mean we should implement CKEDITOR.tools.object.forEach and CKEDITOR.tools.object.reduce functions in tools or just skip them and then use it like:

CKEDITOR.tools.array.forEach( CKEDITOR.tools.object.entries( ... ), ... );

?

[jacekbogdanski]

• Object.keys → CKEDITOR.tools.object.keys

I'm also not happy with the namespace which seems inaccurate. Although, bear in mind that this function is used very often and using alias after changes will create bad looking inconsistency in our code base. Maybe it makes sense to deprecate CKEDITOR.tools.objectKeys and refactor source code to use CKEDITOR.tools.object.keys? Seems like simple grep but may obscure the review.

• Object.values → CKEDITOR.tools.values
• Object.entries → CKEDITOR.tools.entries

If you read the description carefully, you will see that both functions output follows the order of for..in loop. Not sure if it's a deal breaker for us, but it is an important part of the native function. We cannot keep the same order with IE8 polyfill. Nevertheless, different order will be produced for NonEnum props only, so it seems rather like an edge case.

Do you mean we should implement CKEDITOR.tools.object.forEach and CKEDITOR.tools.object.reduce functions in tools or just skip them and then use it like:

CKEDITOR.tools.array.forEach( CKEDITOR.tools.object.entries( ... ), ... );

That would be a bad idea. These functions are used extensively in our code base (or rather reimplemented over and over. They also follow the signature of

object.reduce( obj, function( acc, value, key) {} );

in popular libraries like lodash. As I like making them tuple as an output for simplicity, using:

object.reduce( obj, function( acc, value, key) {
  acc[ key ] = value;
  return acc;
}, {} );

is much more readable than

object.reduce( obj, function( acc, tuple ) {
acc[ tuple[ 0 ] ] = tuple[ 1 ];
return acc;
}, {} );

[Comandeer]

Do you mean we should implement CKEDITOR.tools.object.forEach and CKEDITOR.tools.object.reduce functions in tools or just skip them and then use it like:

I don't have a strong opinion here. Initially I thought about skipping them, but we can also introduce them and use proposed low-level functions inside. However I think that introducing such methods would be slightly redundant.

Although, bear in mind that this function is used very often and using alias after changes will create bad looking inconsistency in our code base. Maybe it makes sense to deprecate CKEDITOR.tools.objectKeys and refactor source code to use CKEDITOR.tools.object.keys? Seems like simple grep but may obscure the review.

Deprecating it can be moved to separate ticket, with only adding new alias in this PR.

I don't understand the part about signatures. What land inside reduce/forEach is based on which method would be used to produce the array. The situation you describe would be caused only by CKEDITOR.tools.object.entries, but using CKEDITOR.tools.object.values would produce more readable callback. What's more, introducing forEach and reduce with signatures different than native ones and different than already introduced forEach and reduce IMO can create much confusion.

TBH the issue with tuple is connected strictly with the fact that we don't use any transpiler and are forced to write directly in ES3. In modern JS the problem is just nonexistent, thanks to destructuring:

Object.entries( obj ).reduce( obj, function( acc, [ key, value ] ) {
	acc[ key ] = value;
	
	return acc;
}, {} );

In ES3 it can be mitigated by some workarounds:

var entries = CKEDITOR.tools.object.entries( obj );

CKEDITOR.tools.array.reduce( entries, function( obj, entry ) {
	var key = entry[ 0 ],
		value = entry[ 1 ];

	obj[ key ] = value;

	return obj;
}, {} );

Maybe not ideal, but still manageable.

[Comandeer]

If you read the description carefully, you will see that both functions output follows the order of for..in loop. Not sure if it's a deal breaker for us, but it is an important part of the native function. We cannot keep the same order with IE8 polyfill. Nevertheless, different order will be produced for NonEnum props only, so it seems rather like an edge case.

Won't it affect only IE8? OTOH I can't think of any case, in which I must depend on properties enumeration order.

[jacekbogdanski]

Won't it affect only IE8?

Yes, I already wrote it in quoted by you comment.

OTOH I can't think of any case, in which I must depend on properties enumeration order.

There may be some cases when using an object as a hashmap, cache, etc. It would be safer to keep the same behavior as a native one. But again, these are only special methods which will be out of order (like toString) - very edge case (like no case at all). Still, exceptions from native implementation may provide some confusion.

I don't understand the part about signatures. What land inside reduce/forEach is based on which method would be used to produce the array. The situation you describe would be caused only by CKEDITOR.tools.object.entries, but using CKEDITOR.tools.object.values would produce more readable callback. What's more, introducing forEach and reduce with signatures different than native ones and different than already introduced forEach and reduce IMO can create much confusion.

There is no native object.reduce/object.forEach, what native signature you are talking about? It has to be different from Array or we will have to play with weird syntax.

However I think that introducing such methods would be slightly redundant.

I would argue, you are mostly using object.reduce or object.forEach every time when you need to call CKEDITOR.tools.objectKeys (of course, using the equivalent implementation in fly) and for .. in loop.

TBH the issue with tuple is connected strictly with the fact that we don't use any transpiler and are forced to write directly in ES3. In modern JS the problem is just nonexistent, thanks to destructuring:

How it's related to the discussion? 🤔 We are not using ecma6 so can't take pleasure of this syntactic sugar.

[f1ames]

I see we didn't reach any conclusion yet? cc @jacekbogdanski @Comandeer

If you read the description carefully, you will see that both functions output follows the order of for..in loop. Not sure if it's a deal breaker for us, but it is an important part of the native function. We cannot keep the same order with IE8 polyfill. Nevertheless, different order will be produced for NonEnum props only, so it seems rather like an edge case.

Since this issue is only present on IE8 and touches only few native object props it's not not a deal breaker here. I would only not that in the method docs that for IE8 there is such inconsistent behavior.

Deprecating it can be moved to separate ticket, with only adding new alias in this PR.

We could do this. But OTOH, deprecating it shouldn't be that much work (marking in docs as deprecated and some replacements in the code) so I think we could carry it in this PR. WDYT?

I don't have a strong opinion here. Initially I thought about skipping them, but we can also introduce them and use proposed low-level functions inside. However I think that introducing such methods would be slightly redundant.

I'm not sure, I also find them kind of redundant (also there are no native alternatives). Are there any particular cases which made you implement these methods or you thought they could be useful in the future? cc @jacekbogdanski

And we already have usages like:

return CKEDITOR.tools.array.reduce( CKEDITOR.tools.objectKeys( widgets ), function( featuredWidgets, id ) { ... } );

[jacekbogdanski]

I already wrote that you could rewrite mostly every for .. in loop to use object.reduce function. I don't say you should, but it should be enough to show you that such function may be used at least to hide deep bracket nesting.

From my perspective, it nicely hides an imperative API using a more declarative approach. In some cases, you also may not have access to the object used for reduction when e.g. using higher order functions. Something like:

( function() {
  var obj = { foo: 1, bar: 2 };
  CKEDITOR.tools.array.reduce( CKEDITOR.tools.objectKeys( obj ), doSomeStuff, {} );
} )();

function doSomeStuff( acc, id ) {
  acc[ id ] = obj[ id ] // Woops! We have a problem. The scope of `obj` object is different.
}

Note that these are tools functions, so mostly they are used when available. It extends your development toolkit, not fixes a particular problem.

For the rest of suggestions like deprecating tools.objectKeys in favor of object.keys and implementing object.entries, object.values we are agreed. I would also refactor tools.objectKeys in the same ticket/PR as a separate commit message - it should be enough to distinguish dump refactoring from implementation for review.

If you still have doubts about object.reduce/forEach I will give up on this one. In my opinion, these functions are essential for smooth declarative programming, but I don't want to waste much time trying to convince you as this case is not super important (although would make life a bit easier).

[jacekbogdanski]

To sum up we will delay object.reduce/forEach API change decision for now as it's not required for any feature ASAP and go with changes proposed with #3123 (comment)