feat: add onlyDefinedProperties option#77
Conversation
…from applying to target.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new skipUndefinedSources option to the deepmerge library that prevents undefined source values from overwriting target properties. This addresses issue #72 by allowing developers to preserve target values when source values are explicitly undefined.
- Added
skipUndefinedSourcesboolean option (defaults to false for backward compatibility) - Modified merge logic to skip undefined source properties when option is enabled
- Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| index.js | Implements the core logic for skipUndefinedSources option with checks at both property and object levels |
| test/skipundefined.test.js | Comprehensive test suite covering flat objects, nested properties, new properties, and array merging scenarios |
| README.md | Documents the new skipUndefinedSources option with usage example |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I am kind of ok, but the name of the option makes me wonder if it is the best. Right now I think the name is not "good" enough. For names I usually want that it is "positive" and not "negative" coined. "skip" is negative coined. I am personally not very convinced of the name. Dont get me wrong, the code is doing what it should do. But this is a module which we very rarely touch. So lets make it kind of perfect, to avoid touching it later again just because the name is confusing. I will ask claude if it has some better names for this option, because I personally cant think of a better one, too. |
|
Claude says
|
|
I think it's still better if it starts with a verb like ignore or omit honestly but just my 2c |
|
This is actually a good counter argument: if it is a verb, than it implies a function. Like isMergeableObject |
|
Yeah in this context it seems like verbs imply functions |
|
Thanks for the feedback, I agree that the option name wasn't great. I like 'onlyDefinedProperties'. |
onlyDefinedProperties option
Uzlopak
left a comment
There was a problem hiding this comment.
LGTM
Thank you for your work. Yeah, naming stuff is sometimes the hardest part in programming :D.
|
@gurgunday |
|
This has yet to be released. Can a new release be cut for this? |
Adds a way to prevent undefined source values from applying to target. See #72
Adding a new
skipUndefinedSourcesoption (but defaulting to false) makes the external API non-breaking.Let me know if anything can be improved.
Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct