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

improve emit types #4817

Merged
merged 13 commits into from
Oct 11, 2023
Merged

improve emit types #4817

merged 13 commits into from
Oct 11, 2023

Conversation

ZachHaber
Copy link
Contributor

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behavior

The types of the emit, emitWithAck, serverSideEmit, and serverSideEmitWithAck have issues with their types.

They revert to any very often or aren't arrays when they should be (server/namespace emit and server/namespace => broadcastOperator emit).

New behavior

This fixes most of this behavior and adds tests for them.

Other information (e.g. related issues)

Fixes #4813

@ZachHaber
Copy link
Contributor Author

Open question:

  • Should the emitWithAck/serverSideEmitWithAck functions only allow events names that have ack functions?

Broadcast emits only return a single ack value
ZachHaber and others added 4 commits September 14, 2023 09:13
Improve type definitions of `emit` and `emitWithAck` to prevent uses in an incorrect manner
This avoids having tons of red squiggles showing on the IDE for a succeeding test
tsd will still error out if a `ts-expect-error` goes unused, so it functionally is about the same
@ZachHaber ZachHaber marked this pull request as ready for review September 14, 2023 22:14
@ZachHaber
Copy link
Contributor Author

ZachHaber commented Sep 14, 2023

@darrachequesne, I've improved the types further, and also removed emitWithAck from the server and namespace.
Some of these type changes will need to be mirrored to the client library to fix up emitWithAck types there. I also can't change the docs website to adjust it to the changes.

I also added in the type-fest library as a dependency. It only has types in it, so it shouldn't impact anything more than npm installing one more package in production. It should be pretty straight-forward to instead inline the 2 types I'm using from the library. Looking at the package-lock file, it's already included about 10 times with nearly as many different versions resolving.

I'll be away from computers from Saturday till October, so feel free to make any needed changes to the PR.

@ZachHaber
Copy link
Contributor Author

@darrachequesne, could you review this when you get a chance?

@darrachequesne
Copy link
Member

@ZachHaber sorry for the delay, I'm taking a look at this.

@@ -241,3 +301,11 @@ export type DecorateAcknowledgementsWithMultipleResponses<E> = {
? (...args: ExpectMultipleResponses<Params>) => Result
: E[K];
};

export type RemoveAcknowledgements<E> = {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure to understand here, shouldn't the events with an acknowledgement be flltered out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It currently just removes the callback function instead of filtering out the events with acknowledgements. It should probably just filter them out in hind sight, shouldn't it?

I was trying to be more permissive than restrictive in what was allowed based on what would technically work.

package.json Outdated
@@ -52,7 +52,8 @@
"debug": "~4.3.2",
"engine.io": "~6.5.2",
"socket.io-adapter": "~2.5.2",
"socket.io-parser": "~4.2.4"
"socket.io-parser": "~4.2.4",
"type-fest": "^3.13.1"
Copy link
Member

Choose a reason for hiding this comment

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

We try to keep our prod dependencies to the bare minimum, so I think we'll indeed need to copy the types from type-fest.

@@ -252,7 +255,10 @@ export class Namespace<
* @return a new {@link BroadcastOperator} instance for chaining
*/
public to(room: Room | Room[]) {
return new BroadcastOperator<EmitEvents, SocketData>(this.adapter).to(room);
return new BroadcastOperator<
DecorateAcknowledgementsWithMultipleResponses<EmitEvents>,
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we move the DecorateAcknowledgementsWithMultipleResponses type to the BroadcastOperator class, so we don't have to apply it everytime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely think that would be nicer; however after looking at the code, I don't think we can.
The BroadcastOperator has an expectSingleResponse flag, which changes the behavior between single responses and multiple. With that, the options I see are to either: apply the decorations when switching to a BroadcastOperator, or to setup the expectSingleResponse as a third generic type parameter on the BroadcastOperator.

@darrachequesne darrachequesne merged commit f6ef267 into socketio:main Oct 11, 2023
6 checks passed
@darrachequesne
Copy link
Member

Awesome work, thanks a lot 👍

@ZachHaber ZachHaber deleted the typescript-emits branch October 11, 2023 14:17
@lts20050703
Copy link

@ZachHaber Awesome work, thank you for your contribution! However, it seems like your PR caused #4914, is there any chance you can look into this?

@ZachHaber
Copy link
Contributor Author

@lts20050703 I'll take a look and see if I can figure that out!

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.

Type issues with emit and emitWithAck
3 participants