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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add support for never type #1154

Merged
merged 1 commit into from Mar 11, 2022
Merged

feat: add support for never type #1154

merged 1 commit into from Mar 11, 2022

Conversation

hmil
Copy link
Contributor

@hmil hmil commented Mar 10, 2022 •
edited by github-actions bot

Fixes #1152

Version

Published prerelease version: v0.98.1-next.0

Changelog

馃帀 This release contains work from a new contributor! 馃帀

Thank you, Hadrien Milano (@hmil), for all your work!

馃殌 Enhancement

馃敥 Dependency Updates

Authors: 2

@hmil hmil changed the title add support for never type feat: add support for never type Mar 10, 2022
@domoritz
Copy link
Member

Can you take a look at #650 and see whether this pull request covers that use case. Note that I reverted the pull request in #668 because it was causing problems.

@domoritz domoritz mentioned this pull request Mar 10, 2022
Closed
@hmil
Copy link
Contributor Author

hmil commented Mar 11, 2022

I can see some similarities between that and what I'm doing here.

Here are some key differences:

  1. In feat: support "any" and "never" within mapped types聽#650, never is formatted as the empty schema, but as I explained in Support the type "never"聽#1152, never should map to the false schema ({"not": {}}), and not to the true schema ({}). It's possible that this mistake caused some of the problems observed with that solution.
  2. feat: support "any" and "never" within mapped types聽#650 modifies the union type formatter, but it doesn't modify the union type itself. In my proposal, never is erased from the union type upon construction, mimicking the true behavior of typescript (where | never is the type algebra equivalent of + 0 in linear algebra). I think that's why I can get away with a simpler solution overall.

In brief, it's not clear to me whether we are solving the same problem, but we are definitely not solving it the same way.

Also worth noting is that my solution may be incomplete:

To really go through with the idea, I should update l.35 of UnionType.ts to return a new NeverType instead of undefined.
However, doing so causes similar test failures as exhibited in this comment, and I believe it's because there are other places where the type algebra would have to be fixed.

The change I propose here while it is not complete, it is also less likely to break existing functionality. If you accept this, I might do a second round to look into cleaning up the type algebra around the null type never thoroughly.

@domoritz domoritz merged commit e1f2449 into vega:next Mar 11, 2022
@github-actions
Copy link

馃殌 PR was released in v0.98.1-next.0 馃殌

@hmil
Copy link
Contributor Author

hmil commented Mar 11, 2022

Thanks! 馃帀

This was referenced Mar 16, 2022
Merged
Merged
Merged
@hydrosquall hydrosquall mentioned this pull request Apr 6, 2022
Closed
@daanboer daanboer mentioned this pull request May 24, 2022
Merged
@daanboer daanboer mentioned this pull request Aug 18, 2022
Merged
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
prerelease
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support the type "never"
2 participants
@hmil @domoritz