Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge to eliminate optionals #30

Closed
thany opened this issue Sep 8, 2023 · 4 comments
Closed

Merge to eliminate optionals #30

thany opened this issue Sep 8, 2023 · 4 comments

Comments

@thany
Copy link

thany commented Sep 8, 2023

Consider this snippet:

interface Foo {
  x: number;
  y: number;
}

const a: Foo = { x: 1, y: 2 };
const b: Partial<Foo> = { x: 3 };

const merged = merge(a, b);

In this snippet, I would expect the values to get merged. They are. So far so good. But I would expect the same for the types. Merging a required and optional member of the same name, should result in that type becoming a required one. That is, among others, what I think merging types should be about.

It should be more than just simply "orring" them together.

@voodoocreation
Copy link
Owner

voodoocreation commented Sep 8, 2023

Hey, this would be a handy feature, but so far I haven't been able to get that working due to not being able to get the types merged in that way so that it can do it more intelligently, with the types of later properties overriding earlier ones (though that gets even more complicated if you've assigned a type to the object that's optional, especially if you explicitly provide a property as undefined in a later rather than omitting it, as the default runtime behaviour is to override it, as it's an explicit value, though we do offer an option to have it not do that), whilst still retaining support for an infinite number of args. I'm not sure if it's a limitation of TS itself or just my knowledge of the more advanced features it has, but I haven't been able to figure out how to get it to work in that way, which is why it does the closest thing I could achieve by detecting all possible property types instead.

If you're working with declared types though, I'd recommend using merge(objA, objB) as Foo; rather than relying on the inferred return types, as your use case is working with known declared types where you do know what the result will be. The return type inferring we provide is most commonly for simpler cases where you're not working with known named types, but rather simple object primitives so that something more useful is inferred from the provided args.

Pull requests are welcomed though if you'd like to have an attempt at getting it working more like what you're expecting/what I've described 🙏🏻

@thany
Copy link
Author

thany commented Sep 8, 2023

To be perfectly honest, I also wouldn't know how to merge types like that. I'm pretty sure it's possible though, perhaps through Narrowing or a similar mechanism.

@voodoocreation
Copy link
Owner

Yeah it's quite a tricky one - especially to get the types to be more aware of all the logic that happens when merging in the runtime code, as well as the various options. This is why the types work as they do currently as a sort of "best guess", referencing all possible types from the merge. You can have a look at how the types work here for context.

I'll leave this issue open for now in case anyone would like to open a PR to address it, so that there's this discoverable context around it.

To work around it though in use cases where you're working with declared types and have the awareness of what it will actually output, I'd definitely recommend using the merge(a, b, c) as MyDeclaredType; syntax so that you can still make use of the deep merge functionality 🙏

@voodoocreation
Copy link
Owner

Actually, I've just added this context to the readme file and linked to this issue, so will close this issue for now, as I'm still not aware of a way for the type inferring to work as described here while still allowing an infinite number of args, as type narrowing is more for inferring the return type from the runtime logic, which isn't how ts-deepmerge performs the return type inferring due to the limitations that has.

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

No branches or pull requests

2 participants