-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #8317 map(...) #8351
Fixes #8317 map(...) #8351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AbdelrahmanHafez . I think this was always a bug, map()
and slice()
create a new instance of Mongoose's custom document array class. It's just that in #8265 we started relying on arrayParentSymbol
when doing operations like push()
and splice()
to ensure that populated()
stays in sync.
I've thought about it some more and I think the way to go is to just add a check in _updateParentPopulated()
that does an early return if parent == null
. If you do docArr.map()
, you get back a new array that shouldn't be tied to the Mongoose document. So I'd remove the custom map()
implementation in favor of just skipping _updateParentPopulated()
if the array doesn't have a parent.
Thank you @Fonger @vkarpov15 You're right though, it makes more sense to return a good old JS array on mapping. Changes coming your way. |
Got some failing tests on Node 4 and 5, will check them soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This should also fix #8312
Will be testing other array methods soon, and fix any broken ones.
@vkarpov15 I am curious, what are the exact changes that introduced this bug?