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

Refactor AMFlowClient #13

Merged
merged 3 commits into from
Apr 3, 2019
Merged

Refactor AMFlowClient #13

merged 3 commits into from
Apr 3, 2019

Conversation

yu-ogi
Copy link
Contributor

@yu-ogi yu-ogi commented Mar 29, 2019

このPullRequestが解決する内容

AMFlowClient をリファクタリングします。

  1. Active の AMFlow#onEvent() 呼び出し前に Passive から送信された Event を保存するように
  2. AMFlow側の仕様 より Error#name を定義に合わせる
  3. すでに stop したプレーに紐付いていた AMFlowClient#close() を呼んだ際にエラーが発生していた件の修正

.then(() => {
activeAMFlow.open(playId);
passiveAMFlow.open(playId);
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

上ですでに open していたので無駄な呼び出しでした。

@@ -422,6 +427,91 @@ describe("AMFlow の動作テスト", () => {
});
});
});

it("AMFlow#onEvent が登録されるより以前の Event を正しく取得できる", done => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

主変更 1. のテストです。

@@ -248,6 +249,7 @@ describe("run-test", () => {
if (err) {
assert.equal(err instanceof Error, true);
assert.equal(permission == null, true);
assert.strictEqual(err.name, "InvalidStatus");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

主変更 2. のテストです。

.then(() => {
return new Promise((resolve, reject) => {
// すでに stop したプレーの AMFlowClient に対して close() を呼び出しても問題ない
passiveAMFlow.close(err => (err ? reject(err) : resolve()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

主変更 3. のテストです。

Copy link
Member

@xnv xnv left a comment

Choose a reason for hiding this comment

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

コメント加えましたが approved.

if (!this.store.isDestroyed()) {
this.store.sendEventTrigger.remove(this.onEventSended, this);
this.store.sendTickTrigger.remove(this.onTickSended, this);
}
this.store = null;
Copy link
Member

Choose a reason for hiding this comment

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

どうせなので unconsumedEvents も捨てませんか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

53500e3 対応しました。

@yu-ogi yu-ogi requested a review from dera- March 29, 2019 08:14
@yu-ogi yu-ogi added the bug Something isn't working label Apr 1, 2019
@yu-ogi yu-ogi merged commit 5a90afc into master Apr 3, 2019
@yu-ogi yu-ogi deleted the refactor-amflow-client branch April 3, 2019 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants