Skip to content
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

Avoid using object union for serde enum serializing #144

Closed
Dreaming-Codes opened this issue Mar 1, 2023 · 2 comments
Closed

Avoid using object union for serde enum serializing #144

Dreaming-Codes opened this issue Mar 1, 2023 · 2 comments

Comments

@Dreaming-Codes
Copy link

Dreaming-Codes commented Mar 1, 2023

Problem

When working with typescript something like the following

#[derive(serde::Deserialize, serde::Serialize, Debug, TS)]
#[ts(export)]
#[serde(tag = "type", content = "data")]
pub enum SignalType {
    #[serde(rename = "add")]
    Add(AddStreamEvent),
    #[serde(rename = "remove")]
    Remove(RemoveStreamEvent),
    #[serde(rename = "ice")]
    ICE(ICEEvent),
    #[serde(rename = "answer")]
    Answer(AnswerOfferEvent),
    #[serde(rename = "offer")]
    Offer(AnswerOfferEvent),
}

#[derive(serde::Deserialize, serde::Serialize, Debug, TS)]
#[ts(export)]
pub struct ICEEvent {
    #[serde(rename = "streamId")]
    stream_id: Option<u8>,
    candidate: String,
}
[...]
[...]
export interface ICEEvent { streamId: number | null, candidate: string, }

export type SignalType = { type: "add", data: AddStreamEvent } | { type: "remove", data: RemoveStreamEvent } | { type: "ice", data: ICEEvent } | { type: "answer", data: AnswerOfferEvent } | { type: "offer", data: AnswerOfferEvent };

could cause some problems in some edge cases like when using the following class

export class TypedEventTarget<EventDef extends { type: any }> extends EventTarget {
    public dispatchEvent(e: Event & EventDef): boolean {
        return super.dispatchEvent(e);
    }

    public dispatch(e: EventDef): boolean {
        const event = Object.assign(new Event(e.type), e);
        return this.dispatchEvent(event);
    }

    public addEventListener<
        T extends EventDef['type'],
        E extends EventDef & { type: T }
    >(type: T, listener: ((e: Event & EventDef) => any) | null) {
        super.addEventListener(type, listener);
    }

    public removeEventListener(type: EventDef['type']) {
        super.removeEventListener(type, null)
    }
}

in this case if I create a class that extend that class like the following

export class WS extends TypedEventTarget<SignalType> {
    private ws: WebSocket;

    constructor(url: string) {
        super();
        this.ws = new WebSocket(url);
        this.ws.addEventListener("message", this.eventHandler);
    }

    public sendEvent(event: SignalType) {
        this.ws.send(JSON.stringify(event));
    }

    private eventHandler(event: MessageEvent<SignalType>) {
        this.dispatch(event.data);
    }
}

there is a problem when using addEventListener since typescript is unable to infer the inner type of SignalType

ws.addEventListener("ice", (event ) => {
	//In this case typescript doesn't know that event is of type Event & { type: "ice", data: ICEEvent }
	//and so it amuses is Event & SignalType (which is not what we want since this is too generic)
})

Solution

To fix this issue, the SignalType type definition can be redefined to be more specific by creating individual types for each event type instead of using a union type. The resulting SignalType type definition would look like the following:

[...]
export interface ICEEvent { streamId: number | null, candidate: string, }

export type AddType =  { type: "add", data: AddStreamEvent };
export type RemoveType =  { type: "remove", data: RemoveStreamEvent };
export type IceType =  { type: "ice", data: ICEEvent };
export type AnswerType =  { type: "answer", data: AnswerOfferEvent };
export type OfferType =  { type: "offer", data: AnswerOfferEvent };

export type SignalType = AddType | RemoveType | IceType | AnswerType | OfferType;
@Dreaming-Codes Dreaming-Codes changed the title Add an option for serde enum serializing Avoid using object union for serde enum serializing Mar 1, 2023
@Brendonovich
Copy link

No comment on whether this should be implemented or not, but I think you have the wrong signature for addEventListener:

public addEventListener<
    T extends EventDef['type']
>(type: T, listener: ((e: Event & Extract<EventDef, { type: T }>) => any) | null) {
    super.addEventListener(type, listener);
}

Playground

@escritorio-gustavo
Copy link
Contributor

Duplicate of #86

@escritorio-gustavo escritorio-gustavo marked this as a duplicate of #86 Jan 26, 2024
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

No branches or pull requests

3 participants