type-fest
type-fest copied to clipboard
Adds type constructors `Patch` and `ReplaceDeep`
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!
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.
We use the name convention where Deep
is at the end. See the other deep types.
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. Replacingundefined
with something other thannull
, 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.
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 :)
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?
Hey @sindresorhus, I requested your review. There are still 2 open questions, would be good to get your take:
- is
Patch
a good name for what this type does - is
undefined
the better default when patching an optional type, or wouldnull
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
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?
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?
is undefined the better default when patching an optional type, or would null be better
@dawidk92 This needs your opinion.
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.
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!
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?
@sindresorhus this is ready for re-review.
To address this question:
I don't want to design types for imaginary use-cases. Can you think of other types than
null
thatundefined
could be replaced with in a real-world situation?
fp-ts
and @effect
codebases would probably want to use None
instead of null
or undefined
to make the outcome more explicit.
I think this does need to be configurable, because patching optional properties and replacing them need to happen in the same pass. Otherwise* once users use ReplaceOptionalDeep
, they don't have any way to recover which properties were optional.
@ahrjarrett Why close? FYI, my lack of response is rather lack of time than lack of interest in this.
fp-ts and @effect codebases would probably want to use None instead of null or undefined to make the outcome more explicit.
👍
any objection to me extracting ReplaceDeep as its own type in a separate PR?
👍
I apologize for not leaving a comment @sindresorhus, I was actually distracted by this issue, which the code in my PR was affected by.
I closed this because I'm not sure what to do about this issue, and did not want it to be accidentally merged until we hear back from the TS team.
I apologize for not leaving a comment @sindresorhus, I was actually distracted by this issue, which the code in my PR was affected by.
It looks like TypeScript 5.3 will have a fix for this, per the playground link in the linked issue's first comment.
I need a ReplaceDeep
type too, for replacing functions with strings. This is so I can JSON.stringify(value)
, when the value might have a function buried deeply in it.
@ajvincent that's great news -- I wasn't expecting such a quick turnaround.
I reopened this PR and will extract ReplaceDeep
this afternoon.
@sindresorhus would you like me to somehow document that the type will suffer the same defect as DeepReadonly
pre-v5.3?
would you like me to somehow document that the type will suffer the same defect as DeepReadonly pre-v5.3?
Yes
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:
- renames
UndefinedToNull
to be justPatch
- 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
How are we looking? I'm reluctant to review a patch that isn't passing all the tests.
I've done the hard part of cleaning this up. Here's a playground so you can interact with it if that's helpful:
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
I did not see an easy way to run eslint with the --fix flag, but maybe I overlooked it.
npx xo --fix
My apologies for not reviewing yet... still failing tests, and GitHub is no longer retaining the logs.