Skip to content

Conversation

@TheSharpieOne
Copy link
Contributor

currently deepExtend is not ensuring the destination is an object when the source is an object.
deepExtend currently does a similar check/set for the case of source being an array.
This PR adds the check for destination being an object and if not sets it to {}.

@brandonrobertz
Copy link

We need a unit test on this IMO

@TheSharpieOne
Copy link
Contributor Author

Tests added for this case, but for a method such as deepExtend... it is seriously lacking tests to the point where adding just this one is almost pointless.

@brandonrobertz
Copy link

Is there a reason we're not using angular.merge?

@TheSharpieOne
Copy link
Contributor Author

idk, I didn't make this thing, just trying to fix it. You can probably create a PR for replacing deepExtend with angular.merge... even go as far as making deepExtend just call angular.merge just so you don't have to replace references to it everywhere.

@brandonrobertz
Copy link

@TheSharpieOne Good idea about the PR. In the meantime I think your fix is good for now. I'm locked at 0.0.8 until I can trust this lib to not break me with a semver patch-version number update.

pablojim added a commit that referenced this pull request Sep 2, 2015
fix(deepExtend): Ensure dest is obj when src is obj
@pablojim pablojim merged commit a8a4e1f into pablojim:master Sep 2, 2015
@pablojim
Copy link
Owner

pablojim commented Sep 2, 2015

Thanks @TheSharpieOne and @brandonrobertz.

pablojim added a commit that referenced this pull request Sep 2, 2015
@pablojim
Copy link
Owner

pablojim commented Sep 2, 2015

angular.merge did not exist when this package was written and only exists in angular 1.4 onwards...

Probably not worth breaking compatibility with lower versions for this one method.

This was referenced Sep 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants