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

typed deepmerge #198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bryan-hunter
Copy link

#179

This deep merges more accurately than #181

I added a typescript test to validate some more complex use cases

@RebeccaStevens
Copy link

RebeccaStevens commented Oct 19, 2020

This is good but it's doesn't handle every case yet.

Problems found:

Case 1:

const a = {
  a: 1,
  b: 'abc',
};
const b = {
  b: 2,
};

const r1 = deepmerge(a, b);

r1 is typed as { a: number; b: string | number; } here where it should be typed as { a: number; b: number; }

Case 2:

interface T {
  a: string;
  b?: string;
}
const t1: T = { a: "foo" };
const t2: T = { a: "bar" };

const r2 = deepmerge(t1, t2);

r2 is typed as { a: string; b: string | undefined; } here but it should be typed as T (which is { a: string; b?: string }). This difference in the types is subtle but can be important - With type T, the key b may not exist on the object, but in typeof r2 the key b must exist on the object.

@RebeccaStevens
Copy link

RebeccaStevens commented Oct 21, 2020

I've edited my above comment as I wasn't initially run in strict mode while test (why isn't strict mode on by default???).

Anywho, @TehShrike I think this MR is good enough to merge in as is. There are slight issues with it and improvements that can be made but this type def is a lot better than what's currently on master.

@TehShrike TehShrike added the ts Typescript-related label Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ts Typescript-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants