Closed dmicic closed 5 years ago
Merging #24 into master will not change coverage. The diff coverage is
100%
.
@@ Coverage Diff @@
## master #24 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 5 5
Lines 56 63 +7
Branches 11 11
=====================================
+ Hits 56 63 +7
Impacted Files | Coverage Δ | |
---|---|---|
src/EventBus.ts | 100% <100%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 23de6fb...964bc0f. Read the comment docs.
Nice work on the PR. I am just wondering if the overhead of having to maintain a second function is worth it.
What about if we try and merge it to a single function: UPDATE: Found solution see below
export function createEventDefinition<P=undefined>(
options?: EventDefinitionOptions<P> | TestPredicateFn<P>
) {
return <T extends string>(type: T) => {
const eventCreator = (payload?: P):{type:T, payload:P} => {
// Allow runtime payload checking for plain JavaScript usage
if (options && payload) {
const testFn = typeof options === "function" ? options : options.test;
if (testFn && !testFn(payload)) {
showWarning(
`${JSON.stringify(payload)} does not match expected payload.`
);
}
}
return {
type,
payload:payload as P
};
};
eventCreator.eventType = type;
eventCreator.toString = () => type; // allow String coercion to deliver the eventType
return eventCreator;
};
}
const event = createEventDefinition()("foo")();
event.payload; // undefined
const event2 = createEventDefinition<{foo:string, bar:number}>()("foo")({foo:'asdas', bar:4});
event2.payload; // {foo:'asdas', bar:4}
Ahh just realised the above wont work with this :( :
const event = createEventDefinition<{foo:string, bar:number}>()("foo")();
so there is this option still not perfect: Found solution see below
export function createEventDefinition<P=undefined>(
options?: EventDefinitionOptions<P> | TestPredicateFn<P>
) {
return <T extends string>(type: T) => {
function eventCreator(payload:void): {type:T}
function eventCreator(payload:P): {type:T, payload:P}
function eventCreator (payload: P | void) {
// Allow runtime payload checking for plain JavaScript usage
if (options && payload) {
const testFn = typeof options === "function" ? options : options.test;
if (testFn && !testFn(payload)) {
showWarning(
`${JSON.stringify(payload)} does not match expected payload.`
);
}
}
if(payload === undefined){
return {type}
}
return {
type,
payload
};
};
eventCreator.eventType = type;
eventCreator.toString = () => type; // allow String coercion to deliver the eventType
return eventCreator;
};
}
const event = createEventDefinition()("foo")();
event.payload; // Property 'payload' does not exist on type '{ type: "foo"; }'
const event2 = createEventDefinition<{foo:string, bar:number}>()("foo")();
event2.payload; // Property 'payload' does not exist on type '{ type: "foo"; }'
const event3 = createEventDefinition<{foo:string, bar:number}>()("foo")({foo:'asdasd', bar: 123});
event3.payload; // {foo:'asdasd', bar: 123}
So as this does look hard to achieve 100% without a separate function we could perhaps wrap the original one? Found a solution See below
export function createEmptyEventDefinition(options?: EventDefinitionOptions<undefined> | TestPredicateFn<undefined>) {
const innerTypeCreator = createEventDefinition<undefined>(options);
return <T extends string>(type: T) => {
const eventCreator = innerTypeCreator(type);
return () => eventCreator(undefined);
}
}
I do feel like this is another example where we need to bend to TypeScript though and it is quite frustrating!
Actually if we use void
it works:
export function createEventDefinition<P=void>(
options?: EventDefinitionOptions<P> | TestPredicateFn<P>
) {
return <T extends string>(type: T) => {
function eventCreator (payload: P) {
// Allow runtime payload checking for plain JavaScript usage
if (options && payload) {
const testFn = typeof options === "function" ? options : options.test;
if (testFn && !testFn(payload)) {
showWarning(
`${JSON.stringify(payload)} does not match expected payload.`
);
}
}
return {
type,
payload
};
};
eventCreator.eventType = type;
eventCreator.toString = () => type; // allow String coercion to deliver the eventType
return eventCreator;
};
}
const event = createEventDefinition()("foo")();
event.payload; // void
const event2 = createEventDefinition<{foo:string, bar:number}>()("foo")(); // Expected 1 arguments, but got 0.
const event3 = createEventDefinition<{foo:string, bar:number}>()("foo")({foo:'asdasd', bar: 123});
event3.payload; // {foo:'asdasd', bar: 123}
Closing this in favour of https://github.com/ryardley/ts-bus/pull/25
Sometimes you just want to publish a event without any any payload. Currently it's possible to use the
createEventDefinition
without the type declaration. However, the event creator method still forces you to pass in some dummypayload
.This PR solves this problem by introducing the function
createEmptyEventDefinition
which sets the generic parameter toundefined
. Not sure whether a generic parameter should beundefined
but at least this way it's obvious that the payload is empty and also the IDE will highlight an error if you typemyevent.payload.property
.Another option would be to introduce another event type without the
payload
property. But that would be too much overhead to maintain...@ryardley What's your view on this?