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

Suggestion: Return nostr event with generics #448

Conversation

antonioconselheiro
Copy link
Contributor

@antonioconselheiro antonioconselheiro commented Oct 23, 2024

Gm,
I'm suggesting to bring back nostr event generic type to kind property and suggesting deprecated the interface named just as Event (to avoid of conflict with Event typescript interface).

This implementation is retrocompatible with no generic NostrEvent because use number as default type value.

If when you finishing trying #447 and be sure that's cool, maybe include this with my suggestion could improve nostr dev experience by having validated kind events like this:

NostrEvent<ShortText | Repost> instead NostrEvent & { kind: ShortText | Repost }

I understand if it get rejected, but I must at least suggest it.

@alexgleason
Copy link
Collaborator

I thought the generic was really valuable and I used it extensively. But after it got deleted from this repo I updated all of my code to remove it. Now I feel like this: https://www.youtube.com/watch?v=2hshkdneE8o

@alexgleason
Copy link
Collaborator

The main reason I accept my fate and didn't fight back against it harder, or even use it in my own library, is because of this TypeScript issue:

let event: NostrEvent;

if (event.kind === 1) {
  // This guard does not work.
  // The type is still `NostrEvent<number>`, not `NostrEvent<1>` as you expect.
  event;
}

As a result of that problem, here is some real code from the Mostr codebase:

switch (event.kind) {
  case 0:
    await handleEvent0(event as NostrEvent<0>);
    return;
  case 1:
    await handleEvent1(event as NostrEvent<1>);
    return;
  case 3:
    await handleEvent3(event as NostrEvent<3>);
    return;
  case 5:
    await handleEvent5(event as NostrEvent<5>);
    return;
  case 6:
    await handleEvent6(event as NostrEvent<6>);
    return;
  case 7:
    await handleEvent7(event as NostrEvent<7>);
    return;
  case 9735:
    await handleEvent9735(event as NostrEvent<9735>);
    return;
}

Since you cannot take advantage of it without casting anyway, much of the value of it is reduced.

In my original version I at least extended the Filter type so requesting the relay would return the correct type of events. I say this is only still valuable if you also do that. But then the complexity increases, and the guards still don't work.

@antonioconselheiro
Copy link
Contributor Author

antonioconselheiro commented Oct 23, 2024

This implementation is compatible to non-generic NostrEvent, so the code with removed generics will not be affected.
About the comparsion guard, this pr #447 has a guard include to solve this, isKind method.

let event: NostrEvent;

if (isKind(event, ShortText)) {
  // Whoah works nicely.
  // The type is `NostrEvent<ShortText>` as you expect.
  event;
}

image

@antonioconselheiro
Copy link
Contributor Author

Suggestion for Mostr code: you can make a record of { [kind: number]: (event: NostrEvent) => Promise<void> }.
So you can just await record[event.kind](event) and remove the switch case complexity.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Oct 23, 2024

I think the idea of it was pretty neat, but in practice it was just annoying 99% of the time.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Oct 23, 2024

This implementation is compatible to non-generic NostrEvent, so the code with removed generics will not be affected.

I don't believe this. I would need overwhelming proof from all the linters and all possible typescript configurations under the sun proving this wouldn't be the case.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Oct 23, 2024

But I don't want you to go out searching for these things, I just want you to assume this will never be merged.

@antonioconselheiro
Copy link
Contributor Author

No problem, I'll keep this in my core stuff

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