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

Add socket.request() as promise wrapper around callbacks #4175

Closed
sebamarynissen opened this issue Nov 18, 2021 · 6 comments
Closed

Add socket.request() as promise wrapper around callbacks #4175

sebamarynissen opened this issue Nov 18, 2021 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@sebamarynissen
Copy link
Contributor

I noticed that in the 4.4.0 release, a timeout flag was added:

socket.timeout(5000).emit("my-event", (err) => {
  if (err) {
    // the client did not acknowledge the event in the given delay
  }
});

Interestingly I implemented similar logic in an application of mine by patching client-side sockets, but I combined it with promises as well. Something like

await socket.request('event', 'some data');

where the promise times out after a fixed amount of time if the server did not acknowledge.

As a result, I was wondering whether it would a nice feature to add to the library as well. The api would look something like

// Awaits indefinitely for a server response
await socket.request('event', 'some data');

// Times out after 5 seconds
await socket.timeout(5e3).request('event', 'some data');

This could be accompanied by a reply() and replyAny() method as well of course, which looks something like this and hence hides the callbacks from the user alltogether:

socket.reply('event', async (data) => {
  return res.toJSON();
});
socket.reply('event', async (...data) => {
  throw new Error('Something went wrong');
});
socket.replyAny(async (event, ...data) => {
  return res.toJSON();
});

Additionally I think it might be useful to have a way of resurrecting errors as well, but I'm not sure how the api can look like. Perhaps something like

socket.errorBuilder(json => buildCustomErrorFromJSON(json));

or

io.errorBuilder(json => buildCustomErrorFromJSON(json));

where socket.errorBuilder can be used to override for that socket.

As always, I'd be happy to create a PR for this should you decide that it is a feature that is desirable to have in the library. In my opinion it fits nicely in the trend that callbacks are more and more being replaced by promises.

@sebamarynissen sebamarynissen added the enhancement New feature or request label Nov 18, 2021
@sebamarynissen
Copy link
Contributor Author

The implementation of the feature can be straightforward:

class Socket {

  constructor() {
    this.buildError = err => this.io.buildError(err);
  }

  errorBuilder(fn) {
    this.buildError = fn;
  }

  request(...args) {
    return new Promise((resolve, reject) => {
      args.push((err, response) => {
        if (err) {
          reject(this.buildError(err));
        } else {
          resolve(response);
        }
      });
    });
  }

  reply(event, handler) {
    this.on(event, (...args) => {
      let cb = args.at(-1);
      if (typeof cb === 'function') {
        cb = args.pop();
      } else {
        cb = (err) => this._error(err);
      }

      let resolve = res => cb(null, res);
      let reject = err => cb(err);

      try {
        let res = handler.call(this, ...args);
        if (res && res.then) {
          res.then(resolve, reject);
        } else {
          resolve(res);
        }
      } catch (e) {
        reject(e);
      }

    });
  }

}

@darrachequesne
Copy link
Member

Yes, that was on my mind too, but then we'd have to include a polyfill for browsers that do not support Promises..

See also: #4144

@sebamarynissen
Copy link
Contributor Author

Wouldn't it be an option to not include a polyfill and leave it up to the user? If the user targets browsers that don't support promises, he would either:

  • Need to polyfill Promise globally
  • Choose not to use request/reply.

@darrachequesne
Copy link
Member

@sebamarynissen that makes sense 👍

Let's keep this issue open, to see if other users want this functionality to be built-in.

@MiniSuperDev
Copy link

I would like this functionality, and it also helps to write code and tests more readable using async await syntax.

But I'm not convinced by the name request, as I understand, the idea is to allow to change the callback syntax of acknowledgement for an await async syntax, so in this way, and without breaking changes and aligned with current naming convention this would be better

  • emitWithAck

and in the same way reply should be onWithAck , although I still see a lot of use for this one, so I appreciate if someone would like to show me an example where it would be useful to use await async when registering event handlers

For example this:

describe("update todo", () => {
it("should update a todo entity", (done) => {
const partialDone = createPartialDone(2, done);
todoRepository.save({
id: "c7790b35-6bbb-45dd-8d67-a281ca407b43",
title: "lorem ipsum",
completed: true,
});
socket.emit(
"todo:update",
{
id: "c7790b35-6bbb-45dd-8d67-a281ca407b43",
title: "dolor sit amet",
completed: true,
},
async () => {
const storedEntity = await todoRepository.findById(
"c7790b35-6bbb-45dd-8d67-a281ca407b43"
);
expect(storedEntity).to.eql({
id: "c7790b35-6bbb-45dd-8d67-a281ca407b43",
title: "dolor sit amet",
completed: true,
});
partialDone();
}
);
otherSocket.on("todo:updated", (todo) => {
expect(todo.title).to.eql("dolor sit amet");
expect(todo.completed).to.eql(true);
partialDone();
});
});

to something like this;

describe('update todo', () => {
  it('should update a todo entity', async () => {
    const todoId = 'c7790b35-6bbb-45dd-8d67-a281ca407b43';
    todoRepository.save({
      id: todoId,
      title: 'lorem ipsum',
      completed: true,
    });

    const todoUpdated = {
      id: todoId,
      title: 'dolor sit amet',
      completed: true,
    };

    const onTodoUpdated = jest.fn();

    otherSocket.on('todo:updated', onTodoUpdate);

    await socket.emitWithAck('todo:update', todoUpdated);

    const storedEntity = await todoRepository.findById(todoId);

    expect(storedEntity).toEqual(todoUpdated);
    expect(onTodoUpdated).toBeCalledWith(todoUpdated);
  });
});

Is there any plan to implement it?

Thank you.

darrachequesne added a commit that referenced this issue Jan 23, 2023
This commit adds some syntactic sugar around acknowledgements:

- `emitWithAck()`

```js
try {
  const responses = await io.timeout(1000).emitWithAck("some-event");
  console.log(responses); // one response per client
} catch (e) {
  // some clients did not acknowledge the event in the given delay
}

io.on("connection", async (socket) => {
    // without timeout
  const response = await socket.emitWithAck("hello", "world");

  // with a specific timeout
  try {
    const response = await socket.timeout(1000).emitWithAck("hello", "world");
  } catch (err) {
    // the client did not acknowledge the event in the given delay
  }
});
```

- `serverSideEmitWithAck()`

```js
try {
  const responses = await io.timeout(1000).serverSideEmitWithAck("some-event");
  console.log(responses); // one response per server (except itself)
} catch (e) {
  // some servers did not acknowledge the event in the given delay
}
```

Related:

- #4175
- #4577
- #4583
@darrachequesne
Copy link
Member

For future readers:

emitWithAck() was added in 184f3cf, included in version 4.6.0.

io.on("connection", async (socket) => {
    // without timeout
  const response = await socket.emitWithAck("hello", "world");

  // with a specific timeout
  try {
    const response = await socket.timeout(1000).emitWithAck("hello", "world");
  } catch (err) {
    // the client did not acknowledge the event in the given delay
  }
});

Have a great day!

@darrachequesne darrachequesne added this to the 4.6.0 milestone Feb 17, 2023
haneenmahd pushed a commit to haneenmahd/socket.io that referenced this issue Apr 15, 2023
This commit adds some syntactic sugar around acknowledgements:

- `emitWithAck()`

```js
try {
  const responses = await io.timeout(1000).emitWithAck("some-event");
  console.log(responses); // one response per client
} catch (e) {
  // some clients did not acknowledge the event in the given delay
}

io.on("connection", async (socket) => {
    // without timeout
  const response = await socket.emitWithAck("hello", "world");

  // with a specific timeout
  try {
    const response = await socket.timeout(1000).emitWithAck("hello", "world");
  } catch (err) {
    // the client did not acknowledge the event in the given delay
  }
});
```

- `serverSideEmitWithAck()`

```js
try {
  const responses = await io.timeout(1000).serverSideEmitWithAck("some-event");
  console.log(responses); // one response per server (except itself)
} catch (e) {
  // some servers did not acknowledge the event in the given delay
}
```

Related:

- socketio#4175
- socketio#4577
- socketio#4583
dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
This commit adds some syntactic sugar around acknowledgements:

- `emitWithAck()`

```js
try {
  const responses = await io.timeout(1000).emitWithAck("some-event");
  console.log(responses); // one response per client
} catch (e) {
  // some clients did not acknowledge the event in the given delay
}

io.on("connection", async (socket) => {
    // without timeout
  const response = await socket.emitWithAck("hello", "world");

  // with a specific timeout
  try {
    const response = await socket.timeout(1000).emitWithAck("hello", "world");
  } catch (err) {
    // the client did not acknowledge the event in the given delay
  }
});
```

- `serverSideEmitWithAck()`

```js
try {
  const responses = await io.timeout(1000).serverSideEmitWithAck("some-event");
  console.log(responses); // one response per server (except itself)
} catch (e) {
  // some servers did not acknowledge the event in the given delay
}
```

Related:

- socketio#4175
- socketio#4577
- socketio#4583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants