Skip to content
This repository has been archived by the owner on Jul 8, 2020. It is now read-only.

Supported Socket.io #51

Open
wants to merge 3 commits into
base: onigo-port
Choose a base branch
from

Conversation

shundroid
Copy link
Member

@shundroid shundroid commented Feb 2, 2017

This change is Reviewable

@shundroid
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 2 at r1 (raw file):

import eventPublisher from "./publisher";
import socketIOClient from "socket.io-client";

S は大文字で、 SocketIOClient かな
new して使っているので


Comments from Reviewable

@shundroid
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


js/sphero-client.js, line 44 at r1 (raw file):

      eventPublisher.publish("acceptName", name);
    });
    this.socket.on("rejectName", () => {

あ、ここ引数で name 受け取り忘れてた!
追加しといてくださいー


Comments from Reviewable

@dadaa
Copy link
Contributor

dadaa commented Feb 2, 2017

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 10 at r2 (raw file):

  }
  connect(wsHost) {
    if (this.socket !== null) return;

ソースを見る限り this.socket に代入するのは、このすぐ下の new SocketIOClient しかないので、
if (!this.socket) return;
で良さそうな気がします。( 数値を扱う変数だとダメだけどね )
constructor の this.socket = null も要らないかな。

あと、this.socket がすでに作られている場合、今渡された引数 wsHost と、すでに this.socket がもっている wsHost が異なる場合、どのような挙動をするのが良いでしょうか?


Comments from Reviewable

@shundroid
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 10 at r2 (raw file):

あと、this.socket がすでに作られている場合、今渡された引数 wsHost と、すでに this.socket がもっている wsHost が異なる場合、どのような挙動をするのが良いでしょうか?

今現在は既に接続があったら何もしていません

サーバーを切り替える、みたいなのがいいと思うんですが、
実装したほうがいいですか?

たぶん上の if 文を消すだけで実装できますw


Comments from Reviewable

@shundroid
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 10 at r2 (raw file):

Previously, shundroid wrote…

あと、this.socket がすでに作られている場合、今渡された引数 wsHost と、すでに this.socket がもっている wsHost が異なる場合、どのような挙動をするのが良いでしょうか?

今現在は既に接続があったら何もしていません

サーバーを切り替える、みたいなのがいいと思うんですが、
実装したほうがいいですか?

たぶん上の if 文を消すだけで実装できますw

あ、それとも Issue にしたほうがいいでしょうか


Comments from Reviewable

@dadaa
Copy link
Contributor

dadaa commented Feb 3, 2017

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 10 at r2 (raw file):

Previously, shundroid wrote…

あ、それとも Issue にしたほうがいいでしょうか

了解です。じゃ、console.log かなにかでログだけでも残しておきましょうか。


Comments from Reviewable

@shundroid
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


js/sphero-client.js, line 10 at r2 (raw file):

Previously, dadaa wrote…

了解です。じゃ、console.log かなにかでログだけでも残しておきましょうか。

そうします、if 文で return する前に console.log をします

@nakamurataichi: お願いします!


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

None yet

3 participants