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

Angular.merge() fails on scope merge #12653

Closed
NickBolles opened this issue Aug 21, 2015 · 5 comments · Fixed by #16036
Closed

Angular.merge() fails on scope merge #12653

NickBolles opened this issue Aug 21, 2015 · 5 comments · Fixed by #16036

Comments

@NickBolles
Copy link

When performing angular.merge() on an object containing a $scope it fails with
"RangeError: Maximum call stack size exceeded"

.Merge() is attempting to deep copy the scope. Which just doesn't work. Angular.copy() states that copying states is not supported.

I was going to submit a PR but i'm not quite sure what the behavior should be. Should it just set a reference to the scope on the new object? (this is what I, as a user, would prefer) or skip it? Whatever it is I don't think it should throw an exception

CodePen:
http://codepen.io/anon/pen/eNwxYj

This has been handled with Dates and RegExp in the following issues:
#12409 #11974 #11720 #10519

@Narretz
Copy link
Contributor

Narretz commented Aug 22, 2015

Difficult case. I think in that case the doctrine applies that the angular.* functions are not universal and should not be adjusted for every use case. Imo We should document this clearly and not support merging scopes.

@NickBolles
Copy link
Author

What would be the behavior of merge then? It would just skip that property? Would it be bad to just set a reference to the scope on the new object? I understand what your saying that the angular.* functions aren't meant to be universal. But it seems like a fairly trivial operation to set a reference to the scope in my opinion.
Like I said I can submit a PR for this. just not quite sure which way to go.

@gkalpak
Copy link
Member

gkalpak commented Aug 26, 2015

IMO, setting a reference to the scope is not the correct behaviour, since merge() is supposed to "deeply extend".
Silently skiping the property, also obscures things.

I would prefer throwing an error (same as angular.copy() when trying to copy Window or Scope objects).

@NickBolles
Copy link
Author

@gkalpak okay. I agree. Setting a reference isn't extending it.
I will put a PR together that documents that .merge() doesn't support scope merging and throw an error.

@lgalfaso
Copy link
Contributor

Adding support for scopes and merge can cause more issues than what it solves. What should happen if the source and the target are scopes? Now, the current error is not the best and would be nice if someone wants to create a PR that would throw a better error when a scope is found

Narretz added a commit to Narretz/angular.js that referenced this issue Jun 6, 2017
This function has problems with special object types but since it's not used in core,
it is not worth implementing fixes for these cases.
A general purpose library like lodash (provides `merge`) should be used instead.

Closes angular#12653
Closes angular#14941
Closes angular#15180
Closes angular#15992
Narretz added a commit to Narretz/angular.js that referenced this issue Jun 6, 2017
This function has problems with special object types but since it's not used in core,
it is not worth implementing fixes for these cases.
A general purpose library like lodash (provides `merge`) should be used instead.

Closes angular#12653
Closes angular#14941
Closes angular#15180
Closes angular#15992
Narretz added a commit that referenced this issue Jun 12, 2017
This function has problems with special object types but since it's not used in core,
it is not worth implementing fixes for these cases.
A general purpose library like lodash (provides `merge`) should be used instead.

Closes #12653
Closes #14941
Closes #15180
Closes #15992
Closes #16036
Narretz added a commit that referenced this issue Jun 29, 2017
This function has problems with special object types but since it's not used in core,
it is not worth implementing fixes for these cases.
A general purpose library like lodash (provides `merge`) should be used instead.

Closes #12653
Closes #14941
Closes #15180
Closes #15992
Closes #16036
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants