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

🎉add feature for deepmergeCustom to have an option that doesn't override values with undefined, default remain overrided #98

Conversation

fatihaziz
Copy link
Contributor

fix #96 and #25

i don't know how to use the type declaration though
@fatihaziz
Copy link
Contributor Author

fatihaziz commented Apr 8, 2022

I'm still not sure about the effect with the merge methods if this option used.
for example,

const deepmergeCustomized = deepmergeCustom({
  enableOverrideUndefinedValues: false,
  mergeOthers: (values, utils) => {
    // will values has undefined in it? is it matter?
  },
  mergeRecord: (values, utils) => {
    // will values has undefined in it? is it matter?
  },
})

or maybe I'm just a little bit overthink it...

@RebeccaStevens
Copy link
Owner

RebeccaStevens commented Apr 9, 2022

I've thought about adding this feature via an option like this before, but there are a few reason why I don't think this way would be best.
The main reason is that it doesn't scale well and custom merges (in this case custom record merging) would have to be able to take into account all the new options.

I think a better way to add this would be via a sub package of predefined custom merges. This will be supported in TS4.7. I'm working on an update to see if I can add this.

@fatihaziz
Copy link
Contributor Author

Ohhh i got it,
All the merge methods has utils option in it so we it should able to do something like (example)
`
mergeRecord: (values, utils) => {
utils.enableUndefinedOverride = true

// then values will has undefined
}
`
In order todo so we need to modify all merge methods which unlikely scallable. I also notice with the implicit use default merge option.

I'm excited to help so I will also think the better way to do it, if possible it still compatible with <4.7 typescript version.

@RebeccaStevens
Copy link
Owner

I've played around with adding a sub-package here: #100

TypeScript <4.7 should still be able to be supported but it will take a little manual work to do so. (Likely a new set of legacy typings for ts >4.0 & <4.7)

@fatihaziz
Copy link
Contributor Author

I got it,
Wait I will try to pull that, and revert my changes.
and try to test it with my 4.6 typescript

@fatihaziz
Copy link
Contributor Author

I will close this PR for now,
as we agree we will use the sub-package solutions.
until we changed our mind

@fatihaziz fatihaziz closed this Apr 10, 2022
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.

Expecting deepmerge-ts default to existing value, instead of overwriting with undefined,
2 participants