Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Suggested alternative for deprecated angular.merge doesn't behave exactly like the deprecated method. #16187

Closed
1 of 3 tasks
absrd opened this issue Aug 21, 2017 · 2 comments
Closed
1 of 3 tasks

Comments

@absrd
Copy link

absrd commented Aug 21, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:

It's more a misguided suggestion in documentation. It is with regards to deprecation of angular.merge and the suggested alternative, Lodash.merge. Lodash doesn't handle defined keys with undefined values, i.e. if I have an object a = {b: 1} and merge it with another object Lodash.merge(a, {b: undefined}) then the result is {b: 1}, which would be {b: undefined} in angular.merge.

Expected / new behavior:

The alternative library should be as close to the deprecated method.

Minimal reproduction of the problem with instructions:

Look above.

AngularJS version: 1.x.y

Browser: all

Anything else:

Suggest another library, e.g: https://www.npmjs.com/package/deepmerge
I'm not associated with the above library other than I'm now using that instead of Lodash.

@frederikprijck
Copy link
Contributor

frederikprijck commented Aug 21, 2017

Interesting, looks like it's idd not the same behavior, see: https://plnkr.co/edit/zVoIVIz52ZAUTUsN133T?p=preview

I guess this should be changed:

* This function is deprecated, but will not be removed in the 1.x lifecycle.

Not sure deepmerge is the best approach as I'm not familiar with it so I can't realy tell.


Note: even tho Object.assign() is not deep cloning, it's interesting to note _.merge is also different from Object.assign when it comes to first level properties.

@gkalpak
Copy link
Member

gkalpak commented Aug 22, 2017

I don't think we need a function that is 100% compatible with angular.merge(). Maybe we should make it more clear that we recommend using a similar (not identical) general purpose merge function and that lodash.merge is a popular example, not necessarily the "officially recommended" replacement.

@Narretz Narretz added this to the Backlog milestone Oct 9, 2017
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 14, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 16, 2019
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants
@Narretz @frederikprijck @absrd @gkalpak and others