Skip to content

Conversation

32bitkid
Copy link
Contributor

@32bitkid 32bitkid commented Apr 5, 2022

edit: @fabienjuif

TODO

  • Update README.md to add factory / type description

A PR candidate to introduce a new feature createBus() and an extension to the typescript definitions to allow strong-typed buses and event filtering. This is just a draft at this point to see if the implementation/feature is worth-while.

Custom bus with createBus()

createBus() creates an independent bus or channel that can be used independently of the global exported useBus and dispatch() methods. Usage:

import { createBus } from 'use-bus';

const [useMyBus, myDispatch] = createBus();

Strongly typed buses with createBus()

createBus() also accepts a generic, which can be used to constrain handler callbacks to strongly-typed event callbacks. Consider:

type BusEvents = 
  |  { type: '@@ui/ADD_ITERATION', duration: string } 
  |  { type: '@@ui/DELETE_ITERATION', idx: number };

const [useBus, dispatch] = createBus<BusEvents>();

useBus('@@ui/ADD_ITERATION', (event) => {
  console.log(event.duration);
}, []);

useBus('@@ui/DELETE_ITERATION', (event) => {
  console.log(event.idx);
}, []);

In the above example, event.duration and event.idx in their handlers will be strongly-typed.

This also works for type predicates as filters functions:

type UIEvents = 
  |  { type: '@@ui/ADD_ITERATION', duration: string } 
  |  { type: '@@ui/DELETE_ITERATION', idx: number };

type DBEvents = 
  |  { type: '@@db/SYNC' } 

type BusEvents = UIEvents | DBEvents;

const isUIEvent = (e: BusEvent): e is UIEvent => e.type.startsWith('@@ui/');

const [useBus, dispatch] = createBus<BusEvents>();

useBus(isUIEvent, (event: UIEvents) => {
  /* do work here */
}, []);

Backwards compatibility

The top-level useBus and dispatch global bus still works the same way as it did before, with a similar type contract for event type shapes.

Possible issues/concerns

  • createBus() supports the same dispatch(string) signature, which would mean that the types on the useBus() handler side might be entirely accurate; missing some fields. There might be a way to constrain that particular helper to only support the short-hand helper when type is the only field defined in the event payload.

@fabienjuif
Copy link
Owner

fabienjuif commented Apr 5, 2022

edit: I am rude, this is once again a great work, this is appreciated 👍

Right now I have the feeling this is overkilled.
I'll try to explain why, I am still opened and if you think I didn't understand what you are trying to do, feel free to explain it to me one more time!

All I want is to keep the lib damn simple, so if we can do it and have what you need, we will be both happy :)

Types definition aside first.

I have the feeling you can avoid collision by using whatever you want as a matcher function and decorate it as I described it here: #5 (comment)

Do we agree that your draft here do not really respond to a new concern, types aside?

About callback types

You don't need to use generic to type your callback argument.

useBus(isUIEvent, (event: UIEvents) => {
  /* do work here */
}, []);

Will already work right?

About filter function type and dispatch type

I can see we have to modify type definition at least in the lib.

Don't you think of something to set types in user code if we add genericity to both of this functions?
Something close to (I know it does not work, but just so you understand what I have in mind):

const myDispatch = dispatch<UIEvent>
const myUseBus = useBus<UIEvent>

Do you really think we need a factory just for that?


PS: is @wobo-jholmes kind of a smurf account or something? 🤔

@32bitkid
Copy link
Contributor Author

32bitkid commented Apr 5, 2022

@fabienjuif So, yea, I'm definitely open for suggestions. Partially why this is a draft rather than going all-in on a proper PR, consider it a placeholder for a discussion. Perhaps a factory is over-kill, however, it does potentially solve one problem, in terms of treating the dispatch/useBus functions as a conjoined tuple that share a type. However, that said, I'm not overly connected to the factory pattern being a requirement, it was more a means to an end.

So, maybe first, lemme explain how I got to here. So, with the current typescript definitions this doesn't quite work (at least with strict typescript enabled). Consider the following, using typescript today.

import useBus from 'use-bus';
import { ReactElement, useState } from 'react';

interface MoveEvent {
  type: 'move';
  position: [number, number];
}

export const Component = (): ReactElement => {
  const [position, setPosition] = useState([0, 0]);
  useBus('bar', (e: MoveEvent) => setPosition(e.position), []);

  return <div>{JSON.stringify(position)}</div>;
};

Which is definitely not happy, with typescript unable to safely coerce the type of EventAction as MoveEvent, which makes sense, they aren't entirely compatible/interchangeable types. So, thats challenge number one. If you just trust that it's a EventAction, then every other property aside from type is type any, which has its own set of concerns and defensive guards you have to implement in the callback handler itself.

  useBus('bar', (e: EventAction) => {
    const actualEvent = e as MoveEvent; /* obviously problematic */
    setPosition(e.position)
  }, []);

Even if that did work, then it would be nice if dispatch was also aware of what shape MoveEvent was, and would throw an compilation error if i gave it a non-conforming object body:

interface MoveEvent {
  type: 'move';
  position: [number, number];
}

dispatch({ type: 'move', x: 0, y: 0 });

So mostly, what i want to do a) strongly define my event types and then b) constrain the dispatch method to make sure that the event bodies I'm emitting match the aforementioned contract. c) not have to cast or guess the callback type, if possible, and let typescript narrow the types down. The factory was just a "clean" way to do that without breaking backwards compatibility for the "generic" global exports that the library presently exposes/others rely on.

So really I want to enforce something like this:

interface UserJoinEvent {
  type: '@@socket/join';
  userId: string;
  name: string;
}

interface UserLeaveEvent {
  type: '@@socket/leave';
  userId: string;
}

interface SomeOtherDomainEvent {
  type: '@@ui/some-other-event';
  stuff: string;
}

// All the event types in the system
type BusEvents = UserJoinEvent | UserLeaveEvent | SomeOtherEvent;

and given that const useBus: UseBus<BusEvents>; then when I bind to that event in a component, to have a strong guarantee of the derived event type.

useBus('@@socket/join', (e) => { 
  /* e is implicitly narrowed down to `UserJoinEvent` automatically; the only event shape that has a matching type */
})

/* and */

dispatch({
  type: '@@socket/join';
  userId: '123';
  name: 'Foo Bar';
}); 

That those are both enforced types against UserJoinEvent.


Tangentially, I considered another option, which is perhaps more palatable. Rather than introduce a factory for solidifying the type for [useBus, dispatch], it was to introduce the UseBus, DispatchFn types into the typescript definitions, and let you just cast in the application domain and say something like:

import defaultUseBus, { dispatch as defaultDispatch, UseBus, DispatchFn  } from 'use-bus';
import { BusEvents } from './my-events';

export const useBus = defaultUseBus as UseBus<BusEvents>;
export const dispatch = defaultDispatch as DispatchFn<BusEvents>;

And that works… But I do have to admit that I, personally, find the createBus() a little easier on the eyeballs, and a little more intentional.

import { createBus } from 'use-bus';
import { BusEvents } from './my-events';

const [useBus, dispatch] = createBus<BusEvents>();
export { useBus, dispatch };

And, yea, I can see how thats really similar to the channels request. And I'm not entirely opposed to this cast approach, but it would be nice to have a "better" way.


ha, no. @wobo-jholmes is a "work", whereas this one is my account. Sometimes I get mixed up between browsers/tabs.

type: string;
export interface EventAction<T extends string = string> {
type: T;
[key: string]: any;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a second optional generic type for the value of [key: string] instead of any and default that new generic type to any

@fabienjuif
Copy link
Owner

fabienjuif commented Apr 6, 2022

Thank you for this detailed response.

I think I've got what you want to achieve when I first respond. However what I didn't have in mind is that code didn't work:

useBus(isUIEvent, (event: UIEvents) => {
  /* do work here */
}, []);

So thank you enlightening me with your response :)

What I have in mind right now, and this is a question because I am not good at typescript as you already noticed I guess, is : Can we then create a type factory?
Can we create something only for typescript that would do what you wrote earlier (a type factory?)?

export const useBus = defaultUseBus as UseBus<BusEvents>;
export const dispatch = defaultDispatch as DispatchFn<BusEvents>;

edit:
I also have the feeling using factories break down the ability to debug events that go through your application.
ie: I can't do something like this anywhere in the application:

useBus(() => true, console.log, [])

We breakdown the single source of truth doing so.
WDYT?

@32bitkid
Copy link
Contributor Author

32bitkid commented Apr 6, 2022

I also have the feeling using factories break down the ability to debug events that go through your application.

Yea, I definitely agree thats a major downside with breaking it out into a factory pattern. I considered having the child buses also emit their dispatched events back into the global bus, but that felt a little too "magical" for what you were going for, in terms of the simplicity of the library.


So, lemme kinda run though some of the alternatives that I considered other than the factory-pattern:

First, there is the aforementioned cast-and-export pattern:

export const useBus = defaultUseBus as UseBus<BusEvents>;
export const dispatch = defaultDispatch as DispatchFn<BusEvents>;

Which I'm not entirely opposed, in fact, it's looking like a better-and-better option at time goes by. Relatively easy to implement on the application side, and relatively simple to document how you use it well.

Another option I considered was to push the generic down to the invocation site, instead of bound to the type. So, rather than casting defaultUseBus as UseBus<BusEvents>, instead each invocation takes a type, where the default is the plain-old EventAction type. Looking something like this:

interface AddEvent {
  type: 'add';
  at: [number, number];
}


interface MoveEvent {
  type: 'move';
  to: [number, number];
}

export type BusEvent = AddEvent | MoveEvent;

and then…

import useBus, { dispatch } from 'use-bus';
import { ReactElement, useState, useEffect } from 'react';
import { BusEvent } from './events';

export const Component = (): ReactElement => {
  const [position, setPosition] = useState([0, 0]);

  useBus<BusEvent>('move', (e) => setPosition(e.to), []);
  useEffect(() => {
    dispatch<BusEvent>({ type: 'add', at: [500, 500] });
  }, []);
  return <div>{JSON.stringify(position)}</div>;
};

A "down-side" of that is that it gets a little chatty, where if you want strongly-typed event-handlers and dispatches, you need remember to apply the generic parameter everywhere you use it. An "upside" is if you don't really care, then you can just use useBus and dispatch the way you normally would and slowly transition to stronger type enforcement as you want:

import useBus from 'use-bus';

export const Component = (): ReactElement => {
  const [position, setPosition] = useState([0, 0]);

  useBus<BusEvent>('move', (e) => setPosition(e.to), []);
  useBus('@@ui/some-other-event', () => console.log('this is fine!'), []);
  return <div>{JSON.stringify(position)}</div>;
};

I don't know of a good way aside from a factory-like pattern, to apply those types as a tuple (to both dispatch and useBus together). Not saying it doesn't exist, I just don't know what it is if it does.

Lastly, as reference point, one that I would prefer to avoid, would be the way that styled-components handles a similar problem with strongly-typing the global theme argument. Which is basically, make a typescript definition styled.d.ts file in your project and manually extend an interface with the properties you want. I don't want to do that.

@fabienjuif
Copy link
Owner

I am glad we understand each other then.

I am taking a look at styled-components solution later, can you tell me quickly (I don't want to loose your time if you don't want to) why you don't want to apply this solution?

@32bitkid
Copy link
Contributor Author

32bitkid commented Apr 6, 2022

I am taking a look at styled-components solution later, can you tell me quickly (I don't want to loose your time if you don't want to) why you don't want to apply this solution?

For sure, so for one, I'm not even entirely sure their direct solution would work in this context, so basically what they are doing is defining, the DefaultTheme

export interface DefaultTheme {
  [key: string]: any;
}

And then extending that shape with a local-module to constrain specific keys in the interface to specific types. But the underlying interface is still valid, so you end up rolling all your definitions into a single derived shape. So you can basically do that as many times as you want:

declare module 'styled-components' {
  export interface DefaultTheme {
    backgroundColor: string;
  }
}

declare module 'styled-components' {
  export interface DefaultTheme {
    textColor: string;
  }
}

is equivalent to just saying:

export interface DefaultTheme {
  [key: string]: any;
  backgroundColor: string;
  textColor: string;
}

…so, you are kinda peeking into another library and adding some goo onto an existing type that it owns, more like an"Intersection Type" than the Union Type that we are looking for; i.e. events are discrete shapes that have a discriminator (type) that can identify what shape it implements.

But mostly my resistance to that pattern comes more from having to override another modules definition in a .d.ts file, just a feels a little weird. Its like poking your head inside node_modules and twiddling some bits to make another libraries types look right.

A pattern closer to what we are looking for is probably the typescript definitions for socket.io clients and server. Their implementation is slightly different, operating on something more like faux-method signatures to define their type arg and callback parameters, that doesn't map entirely well to the event object payload object here. But there are definitely some similar ideas, and where they cast an instance to strongly typed constraints: which would be pretty close the aforementioned cast-and-export like pattern.

const socket: Socket<ServerToClientEvents, ClientToServerEvents> = io();

@fabienjuif
Copy link
Owner

fabienjuif commented Apr 9, 2022

And what do you think of a type factory?
We keep the spirit of your PR but only to cast types. We do not create new instances.

This is the cast-and-export pattern your mentioned earlier but with an helper so we avoid the user to declare its own ugly cast.

In the typescript definition file:

function withTypes<BusEvents extends { type: string }>() {
  return {
    dispatch as Dispatch<BusEvents>,
    useBus as useBus<BusEvents>
  }
}

In user land:

import { withTypes } from 'use-bus';
import { ReactElement, useState, useEffect } from 'react';
import { BusEvent } from './events';

const { dispatch, useBus } = withTypes<BusEvent>()

export const Component = (): ReactElement => {
  const [position, setPosition] = useState([0, 0]);

  useBus('move', (e) => setPosition(e.to), []);
  useEffect(() => {
    dispatch({ type: 'add', at: [500, 500] });
  }, []);
  return <div>{JSON.stringify(position)}</div>;
};

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 this pull request may close these issues.

3 participants