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

fixed update.assign for deep object merges #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AleksandrZhukov
Copy link
Contributor

  • common js Object.assign doesn't have ability to merge objects deeply so added mergeDeep for these needs
  • note: arrays will be replaced with new one

- common js `Object.assign` doesn't have ability to merge objects deeply
  so added `mergeDeep` for these needs
- note: arrays will be replaced with new one
@akuzko
Copy link
Owner

akuzko commented Sep 25, 2019

@AleksandrZhukov sorry for long reply.
a couple issues with this PR:

Initially project was built in pure ES5 with no traspiling, so the code at the moment you opened this PR had to be ES5-compatible. transpiling was added in v1.7.3, so now this has to be just rebased.

It is intended for update.assign to behave exactly like Object.assign, but preserving object immutability. It's hard to show a simple use-case where this might be important, since this is related with deep object nesting, but consider something like this:

const initialTodos = {
  settings: {
    can: {}
  },
  items: []
};

// at some point of time we have something like
data = {
  todos: {
    settings: {
      can: {edit: true}
    },
    items: [todo1],
    valueWeWantToKeep: true
  }
}

// then later on we want to drop portion of `todos` data with

const nextData = update.assign(data, 'todos', initialTodos);

with deep-assign behavior, this code won't drop edit: true value from can object, which also can be pretty unexpected.

what I would actually suggest is to add deepAssign or assignDeep, or preferrably, merge helper with the code that you have implemented, and that will be a great addition to the package

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

2 participants