Skip to content
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

assignDeep copy plain objects values instead of reference #151

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

cherifGsoul
Copy link
Member

For #150

Copy link
Contributor

@justinbmeyer justinbmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this against canjs/canjs? If everything passes, this looks good to me. This feels like something downstream might break.

@cherifGsoul cherifGsoul changed the title deepAssign copy values instead of reference assignDeep copy values instead of reference Nov 29, 2018
@cherifGsoul cherifGsoul changed the title assignDeep copy values instead of reference DON'T MERGE assignDeep copy values instead of reference Dec 4, 2018
@cherifGsoul cherifGsoul changed the title DON'T MERGE assignDeep copy values instead of reference assignDeep copy plain objects values instead of reference Dec 11, 2018
@cherifGsoul
Copy link
Member Author

@justinbmeyer I edited the code and tested it with canjs/canjs and all tests pass, can you please review it?

getSetReflections.setKeyValue(target, key, newVal);
// Plain objects needs to be serialized to make sure
// newVal is copied not referenced #150
if (typeReflections.isMapLike(newVal) && !typeReflections.isObservableLike(newVal)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't solve the original issue since it was using a DefineMap. Is that being handled somewhere else?

@phillipskevin
Copy link
Contributor

@cherifGsoul, can you add tests for each of the scenarios that is failing in other repos?

The tests I can think of:

  • a test for the original issue (assign-deep deep copy)
  • a test to replicate the can-map-define issue where a constructor is passed as a property (assign-deep with a constructor)
  • a test to replicate this cyclic objects test where the same object is passed twice on the source so the same copied object should be on the destination twice (assign-deep with duplicate objects

I think you were also running into an issue with type: "compute", but I'm not sure what it was. That might need a test also.

@phillipskevin

This comment has been minimized.

@cherifGsoul cherifGsoul self-assigned this Dec 19, 2018
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.

None yet

3 participants