type-fest icon indicating copy to clipboard operation
type-fest copied to clipboard

Adds type constructors `Patch` and `ReplaceDeep`

Open ahrjarrett opened this issue 1 year ago • 25 comments

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!

ahrjarrett avatar Jul 15 '23 00:07 ahrjarrett

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.

dawidk92 avatar Jul 17 '23 09:07 dawidk92

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

sindresorhus avatar Jul 17 '23 22:07 sindresorhus

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 avatar Aug 08 '23 06:08 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 avatar Aug 08 '23 06:08 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 avatar Aug 08 '23 07:08 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

ahrjarrett avatar Aug 08 '23 07:08 ahrjarrett

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?

dawidk92 avatar Aug 08 '23 10:08 dawidk92

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 avatar Aug 09 '23 23:08 sindresorhus

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

@dawidk92 This needs your opinion.

sindresorhus avatar Aug 09 '23 23:08 sindresorhus

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.

ahrjarrett avatar Aug 10 '23 02:08 ahrjarrett

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!

dawidk92 avatar Aug 10 '23 09:08 dawidk92

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?

ahrjarrett avatar Aug 10 '23 11:08 ahrjarrett

@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 that undefined 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 avatar Aug 10 '23 11:08 ahrjarrett

@ahrjarrett Why close? FYI, my lack of response is rather lack of time than lack of interest in this.

sindresorhus avatar Sep 02 '23 11:09 sindresorhus

fp-ts and @effect codebases would probably want to use None instead of null or undefined to make the outcome more explicit.

👍

sindresorhus avatar Sep 02 '23 11:09 sindresorhus

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

👍

sindresorhus avatar Sep 02 '23 11:09 sindresorhus

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.

ahrjarrett avatar Sep 02 '23 15:09 ahrjarrett

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 avatar Oct 12 '23 14:10 ajvincent

@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?

ahrjarrett avatar Oct 12 '23 18:10 ahrjarrett

would you like me to somehow document that the type will suffer the same defect as DeepReadonly pre-v5.3?

Yes

sindresorhus avatar Oct 15 '23 11:10 sindresorhus

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

ahrjarrett avatar Nov 20 '23 10:11 ahrjarrett

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

ajvincent avatar Dec 02 '23 23:12 ajvincent

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

ahrjarrett avatar Jan 07 '24 07:01 ahrjarrett

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

npx xo --fix

sindresorhus avatar Jan 07 '24 11:01 sindresorhus

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

ajvincent avatar May 30 '24 05:05 ajvincent