Skip to content

Commit

Permalink
allow multiple client subscriptions
Browse files Browse the repository at this point in the history
  • Loading branch information
nicolodavis committed Oct 15, 2019
1 parent 7f021ab commit 49f5a52
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 10 deletions.
19 changes: 17 additions & 2 deletions src/client/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,25 @@ class _ClientImpl {
}

subscribe(fn) {
const callback = () => fn(this.getState());
this.transport.subscribe(callback);
// If we already have a subscription, then create a new
// callback that invokes both the old and new subscriptions.
const prev = this.subscribeCallback;
const callback = () => {
prev();
fn(this.getState());
};

this.subscribeCallback = callback;
this.transport.subscribe(callback);
callback();

// Return a handle that allows the caller to unsubscribe.

This comment has been minimized.

Copy link
@amitport

amitport Oct 25, 2019

Contributor

@nicolodavis
the unsubscribe behavior is very unexpected and practically does not enable more than 2 subscribers.
it would be simple to just use something like rxjs stream code, but otherwise (if you want to avoid the overhead for now) implementing this properly is also not that difficult, just need to have an object of ids to callback-fn, the unsubscribe function has the id in the closure and removes it from the object (I will have sent a pull request but I don't have time right now, this is already me procrastinating something else)

This comment has been minimized.

Copy link
@nicolodavis

nicolodavis Oct 25, 2019

Author Member

Yes, I agree that this needs to be fixed. I'll try to get to this soon.

Separately, the Debug Panel is now baked into the client itself, so it should greatly simplify the Angular wrapper.

This comment has been minimized.

Copy link
@amitport

amitport Oct 25, 2019

Contributor

@nicolodavis yeah I saw, that looks great! I plan to take a couple of days to update the wrapper. I'm waiting a bit for Angular@9 so I won't need two migrations. Its RC is supposed to be next week and hopefully it will be stable(-ish) in a couple of weeks.

This comment has been minimized.

Copy link
@nicolodavis

nicolodavis Oct 25, 2019

Author Member

Fixed the multiple subscriber problem in c77ba53.

// Warning: Will revert any callbacks that were added
// after this current call to subscribe(), so use it to
// only remove the latest subscription.
return () => {
this.subscribeCallback = prev;
};
}

getState() {
Expand Down
77 changes: 69 additions & 8 deletions src/client/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -536,14 +536,11 @@ describe('log handling', () => {
});

describe('undo / redo', () => {
let game;
beforeEach(() => {
game = Game({
moves: {
A: (G, ctx, arg) => ({ arg }),
},
});
});
const game = {
moves: {
A: (G, ctx, arg) => ({ arg }),
},
};

test('basic', () => {
const client = Client({ game });
Expand All @@ -559,3 +556,67 @@ describe('undo / redo', () => {
expect(client.getState().G).toEqual({ arg: 42 });
});
});

describe('subscribe', () => {
let client;
let fn;
beforeAll(() => {
const game = {
moves: {
A: G => {
G.moved = true;
},
},
};
client = Client({ game });
fn = jest.fn();
client.subscribe(fn);
});

test('called at the beginning', () => {
expect(fn).toBeCalledWith(
expect.objectContaining({
G: {},
ctx: expect.objectContaining({ turn: 1 }),
})
);
});

test('called after a move', () => {
fn.mockClear();
client.moves.A();
expect(fn).toBeCalledWith(
expect.objectContaining({
G: { moved: true },
})
);
});

test('called after an event', () => {
fn.mockClear();
client.events.endTurn();
expect(fn).toBeCalledWith(
expect.objectContaining({
ctx: expect.objectContaining({ turn: 2 }),
})
);
});

test('multiple subscriptions', () => {
fn.mockClear();

const fn2 = jest.fn();
const unsubscribe = client.subscribe(fn2);

expect(fn).toBeCalled();
expect(fn2).toBeCalled();
fn.mockClear();
fn2.mockClear();

unsubscribe();

client.moves.A();
expect(fn).toBeCalled();
expect(fn2).not.toBeCalled();
});
});

0 comments on commit 49f5a52

Please sign in to comment.