feat: Support for async subscriber callbacks

[ffMathy]

Add support for async subscriber callbacks while retaining backwards compatibility.

• I have added automatic tests where applicable
• The PR title is suitable as a release note
• The PR contains a description of what has been changed
• The description contains manual test instructions

Changes

Made it possible to have an async subscriber callback.

Test

Check if subscribers are still called.

[gismya]

Looks great!

Will probably be a few weeks before this can be reviewed and merged. I won't have time to do it before going on summer vacations and the other maintainers are already on vacations.

[gismya]

Looking good. Please add tests for handling both async and non async callbacks.

[ffMathy]

Okay. I looked at the existing tests, and I couldn't find enough inspiration to get started with this easily.

Do you have an idea of where I can start? The existing tests don't seem to have an example that tests the return value of the callback itself, and if that is being replied.

[gismya]

Something like this should work

  test("should handle async callback and return correct data", () => {
    const callback = vi.fn(async () => {
      return new Promise((resolve) => setTimeout(() => resolve("someData"), 1));
    });
    const testEvent = {
      topic: "ftrack.test",
      data: {},
      id: "eventId",
      source: { id: "sourceId" },
    };

    const publishReplySpy = vi
      .spyOn(eventHub, "publishReply")
      .mockImplementation((_, data) => data);

    eventHub.subscribe("topic=ftrack.test", callback);
    eventHub._handle(testEvent);

    setTimeout(() => {
      expect(callback).toHaveBeenCalledWith(testEvent);
      expect(publishReplySpy).toHaveBeenCalledWith(
        expect.anything(),
        "someData",
        expect.anything()
      );
    }, 10);
    publishReplySpy.mockRestore();
  });

  test("should handle sync callback and return correct data", () => {
    const callback = vi.fn(() => "someData");
    const testEvent = {
      topic: "ftrack.test",
      data: {},
      id: "eventId",
      source: { id: "sourceId" },
    };

    const publishReplySpy = vi
      .spyOn(eventHub, "publishReply")
      .mockImplementation((_, data) => data);

    eventHub.subscribe("topic=ftrack.test", callback);
    eventHub._handle(testEvent);

    setTimeout(() => {
      expect(callback).toHaveBeenCalledWith(testEvent);
      expect(publishReplySpy).toHaveBeenCalledWith(
        expect.anything(),
        "someData",
        expect.anything()
      );
    }, 10);
    publishReplySpy.mockRestore();
  });
});

Not the prettiest tests, but without changing the return values we have no way of awaiting everything, so there's a magic 10 ms timeout.

[ffMathy]

I added them now, and they pass. Thank you.

I also fixed formatting from not working on my machine (and not triggering for TS files either), since I didn't have certain CLIs installed. Did this by switching a precommit into using "NPX" instead. Let me know if that's too much, and I'll remove it again.

[gismya]

Didn't actually test the test properly yesterday. It incorrectly passed even when a promise was sent as a response to publishReply, and didn't correctly check the value either. Adding the promises as return value for the function and awaiting (With some other changes) made the tests work as intended.

Good catch with .ts missing from lint-staged. I wonder why you need npx in the command though - could you check that you've done npm install (Or 'yarn' if that's what you're using) and that it's simply not missing the needed node modules? Otherwise I'll investigate if there might be any issues with using npx together with Yarn.

[gismya]

I did some mistakes when line by line suggesting the promise returns. I've commited the correct implementation.

You can remove the changes to package.json - we identified that the commit hooks have more issues and I'm posting a separate PR for that shortly. Thank you for pointing that out! Our IDE setups made us miss that it didn't run correctly.

[ffMathy]

Thanks for this!