Skip to content

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.

4 participants