ryardley / ts-bus

A lightweight JavaScript/TypeScript event bus to help manage your application architecture.
MIT License
135 stars 8 forks source link

Discussion: Explore possible boilerplate reduction in event descriptions #9

Closed ryardley closed 5 years ago

ryardley commented 5 years ago

Despite autocomplete it frustrates me that by using objects for events we have to write the type string twice. Eg.

const taskLabelUpdated = defineEvent<{
  type: "task.label.updated";
  payload: {
    id: string;
    label: string;
  };
}>("task.label.updated"); // <- written again to provide a concrete "string" to the definition function

Could this be solved using classes?

const taskLabelUpdated = defineEventCreator(
  class TaskLabelUpdated extends BusEvent<{
      id:string;
      title:string,
    }> {
    type = "task.label.updated";
  }
);

Is this actually better? It looks like it might actually be more code. Is there a typescript fu way to keep using objects?

dmicic commented 5 years ago

Hey. What about this way of solving the problem?

export function defineEventWrapper<T>(type: string) {  
  const internal = (type: string) => {     
    return defineEvent<{
      type: string,
      payload: T
    }>(type);
  }
  return internal(type);
}

// Usage
export const signInEvent = defineEventWrapper<{
  email: string,
  password: string  
}>("auth.signin");

Would be great to avoid the repetition of the "type". Just find a better name for that function :)

ryardley commented 5 years ago

The issue I suspect that approach may have is that the events that are returned all have a type field that is of type 'string'. That means you cannot use discriminated unions to filter and switch between events. https://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions

That would mean we need to then cast every event by another means.

ryardley commented 5 years ago

I think it should probably be something like this:


class BusEvent<T extends string, P> {
    constructor(public type: T, public payload: P) {}
}

type EventConstructor<T extends string, P> = new (type:T, payload:P)=> BusEvent<T,P>

function defineEvent<T extends string, P>(NewEvent: EventConstructor<T,P>) {
    return (type: T, payload:P) => new NewEvent(type, payload);
}

class FooEvent extends BusEvent<'foo', {foo:string, bar:number}>{}
class BarEvent extends BusEvent<'bar', {plop:string, ding:number}>{}

const fooCreator = defineEvent(FooEvent);
const barCreator = defineEvent(BarEvent);

const fooEvent = fooCreator('foo', {foo:"yes", bar:45});
const barEvent = barCreator('bar', {plop:"yes", ding:45});

function switchOnEvent(event: FooEvent | BarEvent) {
    switch(event.type) {
        case "foo": 
        console.log(event.payload.foo.toLowerCase());
        break;
        case "bar": 
        console.log(event.payload.plop.toLowerCase());
        break;
    }
}
ryardley commented 5 years ago

Pretty sure the above can be simplified - think it is a little too meta as in abstract factory factory creator creator :)

ryardley commented 5 years ago

Actually just realised this does not solve the problem! :(

ryardley commented 5 years ago

I am starting to wonder if there is a way around this at all. Have tried this too:

class BusEvent<P,T=string> {
  type?:T;
  constructor(public payload: P) {}
}

type EventConstructor<P> = new (payload:P)=> BusEvent<P>

function defineEvent<P>(NewEvent: EventConstructor<P>) {
    return (payload:P) => new NewEvent( payload);
}

class FooEvent extends BusEvent<{foo:string, bar:number}>{type:'foo'='foo'}
class BarEvent extends BusEvent<{plop:string, ding:number}>{type:'bar'='bar'}

const fooCreator = defineEvent(FooEvent);
const barCreator = defineEvent(BarEvent);

const fooEvent = fooCreator({foo:"yes", bar:45});
const barEvent = barCreator({plop:"yes", ding:45});

function switchOnEvent(event: FooEvent | BarEvent) {
    switch(event.type) {
        case "foo": 
        console.log(event.payload.foo.toLowerCase());
        break;
        case "bar": 
        console.log(event.payload.plop.toLowerCase());
        break;
    }
}
ryardley commented 5 years ago

Still need to repeat:

class BarEvent extends BusEvent<{plop:string, ding:number}>{type:'bar'='bar'}
ryardley commented 5 years ago

So I have discovered this works but it is verbose:

class FooEvent extends BusEvent<{foo:string, bar:number}>{
  type = 'foo' as const // must remember to use 'as const' here or type checking wont work 
}
ryardley commented 5 years ago

Maybe this can be applied to @dmicic 's or the old method gonna do more experimentation another night

dmicic commented 5 years ago

So I have discovered this works but it is verbose:

class FooEvent extends BusEvent<{foo:string, bar:number}>{
  type = 'foo' as const // must remember to use 'as const' here or type checking wont work 
}

This fails at runtime. Classes in Typescript cannot have const properties. Surprisingly, my editor does not complain.

Nevertheless, I don't find it too verbose but it's prone to errors. If you forget to define the 'as const' on at least one of the classes, the discriminated union filter in your switch case will turn off completely.

dmicic commented 5 years ago

What about this here.

export function defineEvent<T extends BusEvent>() {
    const EventCreatorInternal = (type: T["type"]) => {
        const creator = (payload: T["payload"]) => ({
            type,
            payload
        });
        creator.eventType = type;
        return creator;
    };

    let eventType: Extract<T, "type">;
    return EventCreatorInternal(eventType.type) as EventCreatorFn<T>;
}

const myTypeCreator = defineEvent<{ type: "my.type", payload: {property: number} }>();

No repetition of the type name and it supports discriminated unions as well.

ryardley commented 5 years ago

Except that you are not passing in a string as eventType is undefined. :( TBH I am really not sure it is possible in typescript. I threw this on SO https://stackoverflow.com/questions/57325035/in-typescript-is-it-possible-to-infer-string-literal-types-for-discriminated-uni

dmicic commented 5 years ago

Except that you are not passing in a string as eventType is undefined.

What do you mean exactly? I haven't run the code but VSCode did not complain and was able to resolve the "type" correctly in myTypeCreator. Am I missing something?

Somehow cannot give up on this issue :D

EDIT: Oh, I see. It doesn't transpile...

    let eventType;
    return EventCreatorInternal(eventType.type);
}
ryardley commented 5 years ago

So stack overflow to the rescue and it appears we could fix this issue if we curry the eventDescriptors:

const fooCreator = defineEvent<{foo:string}>()("foo");
const barCreator = defineEvent<{bar:string}>()("bar");

The following checks out:


const defineEvent = <P>() => <T extends string>(type:T) => (payload:P) => ({
  type,
  payload
});

const fooCreator = defineEvent<{foo:string}>()("foo");
const barCreator = defineEvent<{bar:string}>()("bar");

// Example events
const fooEvent = fooCreator({foo:'foo'});
const barEvent = barCreator({bar:'bar'});

// Create a union type to switch over
type AppEvent = ReturnType<typeof fooCreator> | ReturnType<typeof barCreator>;

// Example of switching with a discriminated union
function switchOnEvents(event: AppEvent) {
  switch(event.type){
    case "foo": 
      // compiler is happy about payload having a foo property
      return event.payload.foo.toLowerCase();
    case "bar":
      // compiler is happy about payload having a bar property
      return event.payload.bar.toLowerCase();
  } 
}

I am wondering if some kind of more explicit API might make more sense instead of throwing in what looks like random currying but I am not sure?

const eventCreator = createEventDefinition<{foo:string}>().withType("foo");

in Javascript this would look like this:

const eventCreator = createEventDefinition().withType("foo");

Is redundant currying better than redundant type repetition? I am tempted to say yes(-ish).

dmicic commented 5 years ago

Thanks @ryardley ! I like this solution here.

const fooCreator = defineEvent<{foo:string}>()("foo");
const barCreator = defineEvent<{bar:string}>()("bar");

I think a more explicit API is not really needed. At first glance, the withType() function reminds me of the fluent api/method chaining design where the appending functions are usually optional. And that's not really the case here as the type of the event must be define. However, I am quite agnostic given that the solution without the withType() function does not prevent either from doing the same 'mistake'

ryardley commented 5 years ago

Hmm nothing stopping us from making this work too but without the added ability to do discriminated unions:

// will not switch on discriminated unions
const fooCreator = defineEvent<{foo:string}>("foo");

// will switch on discriminated unions
const fooCreator = defineEvent<{foo:string}>().withType("foo");
dmicic commented 5 years ago

I was referring in my post to your solution

const fooCreator = defineEvent<{foo:string}>()("foo");

and not to my initial proposal

const fooCreator = defineEvent<{foo:string}>("foo");

My understanding is that in the first option above, the discriminated union will work, right?

EDIT: There is no need to implement the 'discriminated union unsupported' version of defineEvent in addition to the supported one.

ryardley commented 5 years ago

Hey @dmicic I created a new Event definition method PR and slapped you on a reviewer and gave you contributor access. Appreciate a review if you have time. If you don't let me know and I will just merge it.

ryardley commented 5 years ago

This is now merged and will be in the next release