- Notifications
You must be signed in to change notification settings - Fork142
Makemeta optional (default toundefined)#94
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coveralls commentedNov 30, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sorry, not an expert in versioning - what should this be bumped to? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This LGTM! don't worry about the versioning :) I'll probably release it as a patch.
Perhaps it would be nice to document the typings in the README? do you think you can take that on as well?
cc@unional in case you have any thoughts.
I'm ok with it. One issue remain is this:microsoft/TypeScript#12400 (comment) That's why I didn't add the default generics yet. You can see type declaration ( |
| * The optional `meta` property MAY be any type of value. | ||
| * It is intended for any extra information that is not part of the payload. | ||
| */ | ||
| meta:Meta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't this supposed to be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nope. That's the whole point. It should not be optional. See#53 and related discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey, sorry to butt in here. Just hit this issue and am a bit confused... Currently I have this issue:
constx:FSA<string,undefined>={type:"foo",payload:"hello"}
Type '{ type: string; payload: string; }' is not assignable to type 'FluxStandardAction<string, undefined>'. Property 'meta' is missing in type '{ type: string; payload: string; }'.So setting the type toundefined does not make the attribute optional.
I think the DefinitelyTyped defs handle thisin a more appropriate way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We discussed that here:#53 (comment)
It is debatable on which is a better solution.
I would say whenmicrosoft/TypeScript#12400 (comment) is resolved, we will be just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah yeah, makes sense. It's just annoying having to add extra code to satisfy types, but a small price to pay I suppose. :)
Okay so unless anyone is opposed I'm going to merge this and release it as a patch today :) |
In related news,#100 would be a prime candidate for another patch like this one... |
No description provided.