Replaced angular.extend with Utilities.extend

[bjarnef]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #7718

Description

This PR replaced of references on angular.extend() with Utilities.extend() except in the Utilities helper itself.

Note that angular.extend(dst, src) could be use with two or more arguments to pass in multiple source objects.
This was used a few places like var object = angular.extend({}, object1, object2); - we could implement this, but not sure if it is worth if as it was only use a few places and if AngularJS is replaced later or we later want to use vanilla JS here.

For example var opts = Utilities.extend({}, options, scope.umbAceEditor); would returns an empty object, while var opts = Utilities.extend(options, scope.umbAceEditor); returns the same as the previous var opts = angular.extend({}, options, scope.umbAceEditor);.

[bjarnef]

Maybe this is one you would like to review @nathanwoulfe and I think the original issue then can be closed 😁🔥🚀

[ronaldbarendse]

For example var opts = Utilities.extend({}, options, scope.umbAceEditor); would returns an empty object, while var opts = Utilities.extend(options, scope.umbAceEditor); returns the same as the previous var opts = angular.extend({}, options, scope.umbAceEditor);.

If that's the case, it's probably a bug in Utilities.extend, as similar methods all update the first object that's passed in and also returns that same object:

• https://docs.angularjs.org/api/ng/function/angular.extend
• https://api.jquery.com/jquery.extend/
• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

[ronaldbarendse]

Looks like Utilities.extend is a facade to angular.extend that only supports 2 parameters:

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/utilities.js

Lines 33 to 37 in 5e4d326

	/**
	* Facade to angular.extend
	* Use this with Angular objects, for vanilla JS objects, use Object.assign()
	*/
	const extend = (dst, src) => angular.extend(dst, src);

So that's the problem with using Utilities.extend({}, options, scope.umbAceEditor); 😉

[bjarnef]

@ronaldbarendse yes, not sure if we wanted to support the format Utilities.extend({}, options, scope.umbAceEditor); since we only used that format a few places in code and if we later swich to another framework or use vanilla JS.

It would require changing the Utilities.extend function.
The source code of angular.extend():
https://github.com/angular/angular.js/blob/master/src/Angular.js#L379-L381

[nathanwoulfe]

Pretty sure boring old Object.assign() allows multiples sources, so `var obj = Object.assign({}, foo, bar) would assign props from foo and bar to the empty object, and return that new object.

Given that, do we need to abstract anything? Could we just use Object.assign instead?

[bjarnef]

@nathanwoulfe it seems Object.assign() doesn't by default do a deep clone https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#deep_clone which angular.extend() didn't support.

Do you know how we can change the Utilities.extend() to support the angular.extend({}, src1, src2) format while still working with angular.extend(dst, src) .. or if we just can handle this with Object.assign() instead?

[markadrake]

@bjarnef @nathanwoulfe I'm definitely not an angular expert - just wondering what the differences are between angular.extend and Object.assign?

https://codesandbox.io/s/polished-grass-61iqt

I did initially pull down the repo, replace everything with Object.assign but who knows if the tests are comprehensive enough to catch any errors that may come from doing this wide search & replace function. That's why I decided against checking anything in. Awesome work you 2!

[nathanwoulfe]

At some point all the angular js facade stuff will disappear, and we'll be using Object.assign everywhere - I'm not sure of use cases where we'd want to be deep-copying (I haven't had a good look, but most cases are extending config objects with additional fields, so Object.assign would be perfectly ok).

Biggest difference between the two methods is .extend is angular-friendly, and preserves the $$hashKey property, which is critical when extending angular-managed objects. If none of our extends are called on angular objects, then that's not an issue.

[nathanwoulfe]

And extending our extend in Utilities is trivial, just needs the spread operator on the src param:

[bjarnef]

@nathanwoulfe I have updated it to use the spread operator.
However I get a different in this. This first opts is the current format of the object. What am I missing?

var opts = angular.extend({}, options, scope.umbAceEditor);
console.log("opts 1", opts);

var opts2 = Utilities.extend({}, options, scope.umbAceEditor);
console.log("opts 2", opts2);

[bjarnef]

I tried with and without spread operator in the Utilities.extend(), but it didn't seem to make a difference on the output here - probably because we only pass in a single source object. 29b107c

[nathanwoulfe]

Hmm, that's interesting - my test case was entirely in the browser console, so maybe different result compared to the gulpified JavaScript?

[bjarnef]

I have reverted back to the original format using an empty destination object {} and added the spread operator to the extend() function, but currently it doesn't work as the "Chrome" theme isn't applied to ace editor.

[bjarnef]

Yes, it could be some configuration in Gulp and/or in Babel.
babel/gulp-babel#142 (comment)

Maybe @nielslyngsoe or @madsrasmussen can help here?

[bjarnef]

@nathanwoulfe okay, I found this https://davidwalsh.name/merge-objects mentioning this:

Note: This syntax is not yet support by all browsers but you can use Babel with the transform-object-rest-spread plugin to enable object merging with the spread operator.

and it seems when configurating this Babel plugin it works: https://babeljs.io/docs/en/babel-plugin-proposal-object-rest-spread

Fixed in 6f3b0f9

[bjarnef]

I have leaved the loose property to the default value false.
Setting useBuiltIns property will use Object.assign directly instead of the Babel's extends helper.

[bjarnef]

@ronaldbarendse @nathanwoulfe are you happy with the latest changes? 😁

[bjarnef]

@nathanwoulfe could you have another look at this? It would be great to solve the last part of your original issue/feature request ✨

[nathanwoulfe]

@bjarnef @ronaldbarendse it took a while, but we got there - I've gone with the alias in utilities:

const extend = angular.extend;

because it's the lowest impact change. No need to modify parameters, it's a direct alias and will Just Work. No traces of angular.extend exist anywhere in the codebase, outside of the Utilities facade. Happy days!

[ronaldbarendse]

@nathanwoulfe I see you've also merged the changes to .babelrc and package.json, which are not needed anymore when using the function alias: probably best to revert these...

[bjarnef]

@ronaldbarendse the changes in .babelrc and package.json let us use spread operators in JavaScript otherwise the the gulp task failed.

[ronaldbarendse]

@bjarnef The spread operator isn't used anymore in this PR, as it now uses the function alias, right? Lets not try to add features/NPM packages that aren't needed to keep everything clean (and not introduce side-effects because of the additional transpiling) 😉