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

Nested unions are "untagged" #61

Closed
OliverJAsh opened this issue Jul 5, 2019 · 4 comments · Fixed by #65
Closed

Nested unions are "untagged" #61

OliverJAsh opened this issue Jul 5, 2019 · 4 comments · Fixed by #65

Comments

@OliverJAsh
Copy link
Collaborator

OliverJAsh commented Jul 5, 2019

I just ran into this bug which resulted in a runtime exception. 😢

import { unionize, ofType, UnionOf } from 'unionize';

const NestedAction = unionize({ Foo: {} }, { value: 'value' });
type NestedAction = UnionOf<typeof NestedAction>;

const Action = unionize({ Nested: ofType<NestedAction>() }, { value: 'value' });

declare const wrong: { value: {} };

// No error!!!
Action.Nested(wrong);

// Expected: (value: { tag: "Foo"; } & { value: {}; }) => …
// Actual: (value: Pick<{ tag: "Foo"; } & { value: {}; }, "value">) => …
// Why is `tag` omitted??
Action.Nested;

One known workaround is to force the nested union tag to be different:

-const NestedAction = unionize({ Foo: {} }, { value: 'value' });
+const NestedAction = unionize({ Foo: {} }, { tag: 'tag2', value: 'value' });

However that's a non-option when modelling Redux actions, since the tag must be type to conform to the usual Redux action type.

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Jul 5, 2019

This change appears to fix it, but I'm probably breaking some other behaviour. Not sure what exactly because it seems there isn't a test for it.

 export declare type Creators<Record, TaggedRecord, TagProp extends string> = {
-  [T in keyof Record]: {} extends Required<UnTagged<Record[T], TagProp>>
+  [T in keyof Record]: {} extends Required<Record[T]>
     ? ((value?: {}) => TaggedRecord[keyof TaggedRecord])
-    : ((value: UnTagged<Record[T], TagProp>) => TaggedRecord[keyof TaggedRecord])
+    : ((value: Record[T]) => TaggedRecord[keyof TaggedRecord])
// error :-)
Action.Nested(wrong);
// no error :-)
Action.Nested(NestedAction.Foo({}));

Related:

@OliverJAsh
Copy link
Collaborator Author

OliverJAsh commented Jul 6, 2019

It seems UnTagged was added to allow the behaviour shown in this test:

https://github.com/sledorze/unionize/blob/9763034266f9b5aae61fcd93ecf8ea70efe7ad11/src/index.spec.ts#L94-L105

https://github.com/pelotom/unionize/pull/36/files#diff-6b623d6bcf5e7f06c466aa060ec9c4b6R94

However this test was removed recently for some reason: 87456d3

Furthermore, I just tried to revive the test and it seems to be throwing a type error.

So, where does this leave us with the issue I described originally?

Either:

  • We still need UnTagged to allow for the behaviour shown in the test. If that's the case, we should 1) revive that test and 2) fix it because that test is now failing.
  • We no longer need UnTagged, as we don't need the behaviour shown in the test. If that's the case, removing it will fix this issue.

@pelotom WDYT?

@pelotom
Copy link
Owner

pelotom commented Jul 14, 2019

I honestly cannot remember what UnTagged was for. Unless @sledorze wants to intercede and make a case for it, I'd say let's get rid of it.

OliverJAsh added a commit that referenced this issue Jul 15, 2019
@OliverJAsh
Copy link
Collaborator Author

#65

pelotom pushed a commit that referenced this issue Jul 15, 2019
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 a pull request may close this issue.

2 participants