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

Allow objects with non-existing keys in PartialDeep #60

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Allow objects with non-existing keys in PartialDeep #60

merged 3 commits into from
Sep 25, 2019

Conversation

merlinnot
Copy link
Contributor

This fixes the following issue (does not compile with latest TypeScript):

interface Interface {
  a: 'a',
}

type PartialInterface = PartialDeep<Interface>;

const value: PartialInterface = {};

@sindresorhus
Copy link
Owner

@kainiedziela Can you review?

@kainiedziela
Copy link
Contributor

I'd say this warrants an extra test case to cover empty objects then.

@@ -39,8 +39,8 @@ expectType<undefined>(partialDeepFoo.bar!.undefined);
expectType<Map<string | undefined, string | undefined> | undefined>(partialDeepFoo.bar!.map);
expectType<Set<string | undefined> | undefined>(partialDeepFoo.bar!.set);
expectType<Array<string | undefined> | undefined>(partialDeepFoo.bar!.array);
expectType<['foo' | undefined] | undefined>(partialDeepFoo.bar!.tuple);
expectType<[('foo')?] | undefined>(partialDeepFoo.bar!.tuple);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for parentheses here? You don't add them in the readonly tuple test (line 46).

@merlinnot
Copy link
Contributor Author

I removed extra parentheses, I'm used to Prettier doing it for me, had to disable it for this repo and didn't notice it.

As for the test case, maybe my example was a little too minimal. Here's another one:

interface Interface {
  a: 'a',
  b: 'b',
}

type PartialInterface = PartialDeep<Interface>;

// Does not compile.
const value: PartialInterface = { a: 'a' };

@merlinnot
Copy link
Contributor Author

I'm not really used to test tools that you guys use, I checked the repositories and it seems it's deprecated, so I wasn't exactly sure what's your approach to this.

I'd appreciate any guidance on what tests would you expect, I'll try to add these.

@sindresorhus
Copy link
Owner

We use https://github.com/SamVerschueren/tsd. It's not deprecated.

@merlinnot
Copy link
Contributor Author

Oh, I thought that's the one: https://github.com/DefinitelyTyped/tsd. First result in Google :)

Thanks for the link, I'll try to add some tests tomorrow.

@kainiedziela
Copy link
Contributor

I'd say you can just follow the pattern already established in the present type-fest test cases. The library used is pretty intuitive.

@kainiedziela
Copy link
Contributor

@merlinnot I'd be williing to help out (seeing as this is basically an error on my part) if you want. I'd need to be authorized to access your branch to prevent multiple pull requests.

@merlinnot
Copy link
Contributor Author

@kainiedziela I sent you an invite. I was playing with it a little but the only test case I could come up with is to basically paste the code above to the test file, which I guess is not what's expected.

I also changed the existing tests to make them pass, so it's already a regression test of sorts.

@kainiedziela
Copy link
Contributor

No, I think that would be quite intuitive. I pushed the test cases that should cover our needs. I also removed the version change as I believe that should be handled by package maintainers.

@sindresorhus this looks good to go.

@sindresorhus sindresorhus merged commit 1b34d7e into sindresorhus:master Sep 25, 2019
@sindresorhus
Copy link
Owner

Thanks for fixing :)

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

Successfully merging this pull request may close these issues.

3 participants