Skip to content

feat: add onlyDefinedProperties option#77

Merged
Uzlopak merged 4 commits intofastify:mainfrom
MishaKanai:main
Sep 14, 2025
Merged

feat: add onlyDefinedProperties option#77
Uzlopak merged 4 commits intofastify:mainfrom
MishaKanai:main

Conversation

@MishaKanai
Copy link
Contributor

Adds a way to prevent undefined source values from applying to target. See #72

Adding a new skipUndefinedSources option (but defaulting to false) makes the external API non-breaking.

Let me know if anything can be improved.

Checklist

@Uzlopak Uzlopak requested a review from Copilot September 11, 2025 07:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 skipUndefinedSources boolean 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.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 11, 2025

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.
Also is it really about undefined Sources(plural?). I personally had too many issues in the past regarding such naming issues. :D

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.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 11, 2025

Claude says

Looking at the code, I can see that `skipUndefinedSources` controls whether properties with `undefined` values from the source object are ignored during merging. You're right that the name could be more positive and clearer.

Here are some better alternatives:

**Positive, clear options:**
- `ignoreUndefined` - Simple and positive
- `omitUndefined` - Clear about what gets left out
- `excludeUndefined` - Similar to omit but more explicit
- `requireDefinedValues` - Emphasizes that only defined values are included

**More descriptive options:**
- `onlyDefinedProperties` - Makes it clear what gets processed
- `definedPropertiesOnly` - Alternative phrasing of above
- `includeDefinedOnly` - Positive framing of what gets included
- `filterUndefined` - Suggests filtering out undefined values

**Property-focused options:**
- `skipUndefinedProps` - Shorter than "Sources" and more accurate
- `ignoreUndefinedProps` - Positive version of skip

My top recommendations would be:
1. **`ignoreUndefined`** - Short, positive, and immediately clear
2. **`onlyDefinedProperties`** - More verbose but very explicit about behavior
3. **`omitUndefined`** - Clear and commonly understood in programming contexts

The current name `skipUndefinedSources` is indeed confusing because:
- "Sources" implies multiple source objects, but it applies to properties within sources
- "Skip" is negative framing
- It's not immediately clear what happens when you "skip" them

What do you think of these alternatives?

onlyDefinedProperties seems good, what do you think?

@gurgunday
Copy link
Member

I think it's still better if it starts with a verb like ignore or omit honestly but just my 2c

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 13, 2025

This is actually a good counter argument:

if it is a verb, than it implies a function. Like isMergeableObject

@gurgunday
Copy link
Member

Yeah in this context it seems like verbs imply functions

@MishaKanai
Copy link
Contributor Author

Thanks for the feedback, I agree that the option name wasn't great. I like 'onlyDefinedProperties'.

@Uzlopak Uzlopak changed the title Add 'skipUndefinedSources' option feat: add onlyDefinedProperties option Sep 14, 2025
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you for your work. Yeah, naming stuff is sometimes the hardest part in programming :D.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 14, 2025

@gurgunday
Can you please release as minor :)?

@Uzlopak Uzlopak merged commit aa01378 into fastify:main Sep 14, 2025
14 checks passed
@Swiftwork
Copy link

This has yet to be released. Can a new release be cut for this?

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.

5 participants