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.

Sign up for GitHub

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

Jump to bottom

feat: support empty mapped types #1261

Merged
merged 2 commits into from
May 26, 2022
Merged

feat: support empty mapped types #1261

merged 2 commits into from
May 26, 2022

Conversation

daanboer
Copy link
Contributor

@daanboer daanboer commented May 24, 2022 •
edited by github-actions bot
Loading

Currently, types such as:

export type MappedNever = {
  [K in never]?: never;
};

and

export type MappedExclude = {
  [K in Exclude<"key", "key">]?: never;
};

Result in the respective errors :

Error: Unexpected key type "never" for type "{ [K in never]?: never }" (expected "UnionType" or "StringType")

and

Error: Unexpected key type "undefined" for type "{ [K in Exclude<"key", "key">]?: never }" (expected "UnionType" or "StringType")

Instead, both scenarios should result in an empty object. This change simply adds the needed clauses to handle these types in MappedTypeNodeParser.

Ideally, both cases are handled in a single clause, as the second example should be equal to the first (both result in a never key type). This is currently not the case, since resolving an empty union results in undefined instead of new NeverType(), as mentioned in #1154 (comment). However, properly propagating NeverType is hard as it results in regressions, e.g. see #650 (review). The second clause can probably be removed once these issues are solved.

Version

Published prerelease version: v1.1.0-next.5

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ null@daanboer

❤️ null@filipomar

❤️ Remi Cattiau (@loopingz)

❤️ Hadrien Milano (@hmil)

🚀 Enhancement

🐛 Bug Fix

🔩 Dependency Updates

Authors: 9

@domoritz
Copy link
Member

Can you add a test case?

@daanboer
Copy link
Contributor Author

I added two test cases, one for either example.

@domoritz domoritz merged commit 0b04c7d into vega:next May 26, 2022
@domoritz
Copy link
Member

Thank you

@daanboer daanboer mentioned this pull request Aug 18, 2022
Merged
This was referenced Sep 21, 2022
Merged
Merged
@github-actions
Copy link

🚀 PR was released in v1.1.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daanboer @domoritz