-
Notifications
You must be signed in to change notification settings - Fork 15
considering strongly-typed bus events. #33
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
base: master
Are you sure you want to change the base?
Conversation
edit: I am rude, this is once again a great work, this is appreciated 👍 Right now I have the feeling this is overkilled. 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 typesYou 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 typeI 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? 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? 🤔 |
@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 So, maybe first, lemme explain how I got to here. So, with the current typescript definitions this doesn't quite work (at least with 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 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 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 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 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 Tangentially, I considered another option, which is perhaps more palatable. Rather than introduce a factory for solidifying the type for 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 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 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; |
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.
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
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? export const useBus = defaultUseBus as UseBus<BusEvents>;
export const dispatch = defaultDispatch as DispatchFn<BusEvents>; edit: useBus(() => true, console.log, []) We breakdown the single source of truth doing so. |
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 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 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 Lastly, as reference point, one that I would prefer to avoid, would be the way that |
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? |
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 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 ( But mostly my resistance to that pattern comes more from having to override another modules definition in a A pattern closer to what we are looking for is probably the typescript definitions for const socket: Socket<ServerToClientEvents, ClientToServerEvents> = io(); |
And what do you think of a type factory? This is the 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>;
}; |
edit: @fabienjuif
TODO
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 exporteduseBus
anddispatch()
methods. Usage:Strongly typed buses with
createBus()
createBus()
also accepts a generic, which can be used to constrain handler callbacks to strongly-typed event callbacks. Consider:In the above example,
event.duration
andevent.idx
in their handlers will be strongly-typed.This also works for type predicates as filters functions:
Backwards compatibility
The top-level
useBus
anddispatch
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 samedispatch(string)
signature, which would mean that the types on theuseBus()
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 whentype
is the only field defined in the event payload.