Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Add event batching on the server #8

Merged
merged 3 commits into from
Nov 13, 2017
Merged

Conversation

ganemone
Copy link
Contributor

@ganemone ganemone commented Nov 13, 2017

Fixes #7

@ganemone ganemone self-assigned this Nov 13, 2017
@ganemone ganemone changed the title Batch events on the server Add event batching on the server Nov 13, 2017
@lhorie
Copy link
Contributor

lhorie commented Nov 13, 2017

Need docs

@nadiia
Copy link
Contributor

nadiia commented Nov 13, 2017

Please update the readme

@@ -114,7 +133,7 @@ Registers a callback to be called when an event of a type is emitted. Note that
#### `events.emit`

```js
events.emit(type, payload, ...args)
events.emit(type, payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like the ctx should go as an optional arg here? line 141 also needs to be updated

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 actually don't allow emitting events with ctx as a third argument. It is added by using EventEmitter.of(ctx)

@ganemone
Copy link
Contributor Author

!merge

@old-fusion-bot old-fusion-bot bot merged commit 4b8ae44 into fusionjs:master Nov 13, 2017
for (let index = 0; index < items.length; index++) {
const {type, payload} = items[index];
emitter.emit(type, payload);
}
ctx.status = 200;
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter that we are now updating status downstream here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I think we should set it as early as possible

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants