Adds type constructors Patch and ReplaceDeep

[ahrjarrett]

Closes #641

The implementation includes a generic, perhaps more useful type DeepReplace that I could separate into its own module in a separate PR if you want.

Also note that DeepUndefinedToNull and DeepReplace both handle unions the way you'd expect.

Not 100% sure about the name DeepUndefinedToNull though, since in practice it does more than that (since replacing partial properties with null is configurable, and since it also functions as a kind of DeepRequired.

Happy to make any requested changes!

[dawidk92]

It works as expected @ahrjarrett ! 🎉

I appreciate the effort you've put into this. About the naming, DeepUndefinedToNull could potentially be misleading given that the functionality extends beyond just replacing undefined with null.

One idea could be to rename it to DeepReplaceUndefined<Type, Replace>, which might better encapsulate its actual function. However, I'm still trying to grasp the practical scenarios for this use case. Replacing undefined with something other than null, especially with a basic type like a string, seems to be a less likely scenario.

Before we move forward with the name change, could you elaborate on potential scenarios where users might find this beneficial? I'm keen to get your thoughts on this.

[sindresorhus]

We use the name convention where Deep is at the end. See the other deep types.

[sindresorhus]

We don't use the * prefix for doc comments. See other types here.

[ahrjarrett]

One idea could be to rename it to DeepReplaceUndefined<Type, Replace>, which might better encapsulate its actual function. However, I'm still trying to grasp the practical scenarios for this use case. Replacing undefined with something other than null, especially with a basic type like a string, seems to be a less likely scenario.

Could you tell me more about your use case? To be honest I picked this up because it sounded fun, but I'm still a little fuzzy on where this type might be useful.

At first I figured this would be useful because JSON doesn't support undefined as a value, but your ask (if I understood it correctly) was to only "patch" partial properties with null, and otherwise leave the rest of the type alone. If that's a misreading on my part, let me know and I'll change the function to work that way -- in fact, most of the complexity of this type involved leaving other undefined values alone, and only replacing optional properties with null (which is why the Placeholder stuff is in there).

Before we move forward with the name change, could you elaborate on potential scenarios where users might find this beneficial? I'm keen to get your thoughts on this.

I mean it depends on the use case right -- I've worked some places where undefined properties are stripped away entirely (the worst option, IMO), and I've worked places where something like { _tag: "None" } is used in the absence of a value, which makes things a little more explicit.

The reason I was thinking to support that kind of configuration is because this is a lossy transformation. By which I mean, once the optional properties are patched, we can't recover which properties were optional. You have to do both in one pass: 1) patch the optional properties, and 2) replace them with some other type.

[ahrjarrett]

As far as what to name this type, I've been kicking it around, and I keep coming back to Patch and PatchDeep.

According to MW a patch is

a piece of material used to mend or cover a hole or a weak spot

I think what we're doing boils down to exactly that: we're fixing a hole or a weak spot :)

[ahrjarrett]

Also last thought (and then I'm done blowing up this PR) -- I'm actually leaning towards undefined being the default replacement value.

@dawidk92 thoughts?

[ahrjarrett]

Hey @sindresorhus, I requested your review. There are still 2 open questions, would be good to get your take:

1. is Patch a good name for what this type does
2. is undefined the better default when patching an optional type, or would null be better

Besides those questions, I wanted to make sure there wasn't anything else you wanted changed, that way once we decide, all that's needed is a name change / default argument change

[dawidk92]

Thank you all for the hard work and the thoughtful discussion around this topic.

@ahrjarrett , the reason I'm interested in this type is related to my work with GraphQL on my backend. In GraphQL, when something is undefined, it gets returned as null. This often leads to mismatches between my TypeScript types and the actual runtime values. By having a type that systematically replaces undefined with null, I can ensure that my types are aligned with what actually happens at runtime.

Regarding the name, following the convention mentioned by @sindresorhus , I agree that placing 'Deep' at the end is consistent with the other types in the library. I propose the name ReplaceUndefinedDeep. This name clearly describes what the type does and follows the existing naming convention.

I also think it might be wise to require the user to explicitly specify the value they want to replace undefined with. Although replacing with null should be common in many contexts, there might be cases where a different replacement value is more appropriate. Making the replacement value an explicit parameter (e.g., ReplaceUndefinedDeep<MyCustomType, null>) adds flexibility and prevents unexpected behavior for those who might want to replace undefined with something other than null.

Your thoughts, @ahrjarrett, @sindresorhus?

[sindresorhus]

I also think it might be wise to require the user to explicitly specify the value they want to replace undefined with. Although replacing with null should be common in many contexts, there might be cases where a different replacement value is more appropriate. Making the replacement value an explicit parameter (e.g., ReplaceUndefinedDeep<MyCustomType, null>) adds flexibility and prevents unexpected behavior for those who might want to replace undefined with something other than null.

I don't want to design types for imaginary use-cases. Can you think of other types than null that undefined could be replaced with in a real-world situation?

[sindresorhus]

is undefined the better default when patching an optional type, or would null be better

@dawidk92 This needs your opinion.

[ahrjarrett]

I could have done a better job communicating, let me give an example:

The reason I don't think UndefinedToType is a good name is because if that's all someone needed, they could use a more general purpose type ReplaceDeep (in this PR) to accomplish that.

But ReplaceDeep can be used in other scenarios -- for example, here's how it could be used in a different way.

If that all this type did was replace undefined with another type, my recommendation would be to let users define their own replacements ad hoc, and just expose something like ReplaceDeep (that way the API doesn't get bloated, and the broadest set of use cases are supported).

The reason that won't work here is because the main thing that this type does is not replace undefined with another value. Rather, it "fixes" a schema so none of its properties are optional anymore.

That's why I proposed the name Patch (or PatchDeep in this case).

This fundamentally changes the structure of the type, since all of its properties are guaranteed to be present. The reason this is not a trivial operation is because you lose which fields were patched as soon as you perform that operation.

That's why I think this type addresses a real need, is because doing both at the same time is tricky. Using a type like PatchDeep gives users a "hook", so both operations can happen at the same time.

[dawidk92]

Thank you, @sindresorhus and @ahrjarrett, for your insights.

@sindresorhus, I understand the need to avoid designing types for imaginary scenarios. Let me provide more context on my use-case, as it's a real-world need. In my backend development with GraphQL, fields that are undefined are serialized as null when sent to the client. It's vital for my TypeScript types to accurately represent this behavior, ensuring that the types align with the actual data structure sent to the client. This is why I'm interested in a type that can replace undefined with null. It's not merely an aesthetic or theoretical requirement but directly impacts the consistency and reliability of my codebase.

Regarding your specific question about whether undefined or null would be the better default when patching an optional type, I lean towards undefined. It seems to be a more truthful representation of an optional property, as it's closer to the absence of a value, whereas null might imply an intentional assignment of "no value."

@ahrjarrett, your proposal for PatchDeep makes sense in terms of fixing a schema to make properties non-optional. It occurs to me that for my specific use case, I could first use PatchDeep to replace optional properties with undefined, and then use ReplaceDeep to replace those undefined values with null. This two-step approach would align the types with the runtime behavior of GraphQL.

However, I must admit that I find the proposed names Patch and PatchDeep slightly less intuitive. They don't immediately convey the action of replacing optional properties. What about something like ReplaceOptionalDeep? It might be more self-explanatory and aligns well with the existing ReplaceDeep functionality.

I appreciate all the work and thought going into this. Please let me know if there's anything else I can do to assist or clarify!

[ahrjarrett]

ReplaceOptionalDeep works for me. I agree that undefined is probably the less surprising default.

@sindresorhus any objection to me extracting ReplaceDeep as its own type in a separate PR?

[ajvincent]

Consider writing JSDoc now?

[ajvincent]

Bikeshedding comments on the naming here. 😁

Suggestion: "Needle", "Haystack"? They're pretty common terms for search/replace.

Also, "Replace" versus "Replacement"? This project has a "Replace" type already. I wouldn't like to see confusion.

[ajvincent]

Peanut gallery observation: visually verifying this code is going to require scrolling up and down a lot, particularly if this grows more complex in the future. I would recommend moving the declares and expectTypes closer to the types you're actually testing. But if the project owners disagree, I would go with their recommendation.

Not-so-peanut-gallery observation: what happens if someone puts the find type inside the replacement type? Let's come up with some evil replacement testcases too, including:

• complex types for the replace value
• never
• unknown
• any
• a value already in the type

[ahrjarrett]

@ajvincent thank you for your review.

I appreciate you putting thought into how recursion could misbehave if the replacement itself contained part(s) that matched the needle. The nice thing about implementing find/replace this way (handling a positive match as a base case) is that you avoid that complexity altogether.

I added extra tests for Patch, which I'll be requesting your review on in the next day or 2. But wanted to answer your concern while it was fresh on my mind -- since a match is treated as a base case, and we don't continue recursing, users can use any type they like: any, never, a copy of the entire tree -- because we consider that to be the base case, we will never traverse down that subtree.

[ahrjarrett]

I pushed up some changes. I still have to go back and rewrite things to A) remove old code, and B) rewrite things to be consistent with the rest of type-fest.

The main changes include:

1. renames UndefinedToNull to be just Patch
2. uses better names (thank you @ajvincent)

I'm pretty happy with how Patch turned out, API-wise.

I'll finish #1 and #2 hopefully tomorrow after work, but wanted to push something up to make it clear that I haven't abandoned this work

[ajvincent]

How are we looking? I'm reluctant to review a patch that isn't passing all the tests.

[ahrjarrett]

I've done the hard part of cleaning this up. Here's a playground so you can interact with it if that's helpful:

Playground

I still need to figure out how to (hopefully programmatically) clean up the linting errors (over 700 of them). I did not see an easy way to run eslint with the --fix flag, but maybe I overlooked it.

Before I spend the time to do that, it would be helpful to get some feedback so I don't have to do it twice.

Edit: tagging @sindresorhus @ajvincent

[sindresorhus]

I did not see an easy way to run eslint with the --fix flag, but maybe I overlooked it.

npx xo --fix

[ajvincent]

My apologies for not reviewing yet... still failing tests, and GitHub is no longer retaining the logs.

[sindresorhus]

Closing as this is not moving forward.