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

Allow passing options to server when creating the connection. #41

Merged
merged 4 commits into from Oct 28, 2018

Conversation

amit-gshe
Copy link
Contributor

No description provided.

@seiyria
Copy link

seiyria commented Oct 27, 2018

  • what purpose does this actually serve? what options are you passing in here that you wouldn't pass in when joining etc?
  • why is the .d.ts entirely changed when only one line changed? might want to make sure your diff is being reflected accurately
  • protected connect(colyseusid: string,options: any = {}) { you might want to add a space after the comma there (for lint purposes)

@amit-gshe
Copy link
Contributor Author

@seiyria ,

* what purpose does this actually serve? what options are you passing in here that you wouldn't pass in when joining etc?

The server has an verifyClient option, so we can verify the client when it connected to server and before they trying joining the room. Now we have no way to add some request parameters such as user token when the first connection to the server. By the way, the first connection to the server can be used as a main connection and we can use to communicate with client (by some other Protocol). Yes we can add them to a chat room but this way needs to add an another expensive connection to the chat room.

* why is the .d.ts entirely changed when only one line changed? might want to make sure your diff is being reflected accurately

* `    protected connect(colyseusid: string,options: any = {}) {` you might want to add a space after the comma there (for lint purposes)

Sorry for these two mistakes and should I give an another pull request?

@seiyria
Copy link

seiyria commented Oct 28, 2018

You should just commit to the branch for this pr to update it.

As for passing options, is it not sufficient to pass them in client.join()?

@amit-gshe
Copy link
Contributor Author

In some use cases, such as that we need to maintain a message channel to the verified user out of the room or the user may not join a room immediately, the options pass by client.join() is not sufficient.

@amit-gshe
Copy link
Contributor Author

amit-gshe commented Oct 28, 2018

@seiyria

* why is the .d.ts entirely changed when only one line changed? might want to make sure your diff is being reflected accurately

The line ending of colyseus.d.ts is CRLF and other files in this repository are LF. Should I change it back to CRLF?

@seiyria
Copy link

seiyria commented Oct 28, 2018

Yes, you should strive to make your diff as minimal as possible to ensure ease of review.

@endel endel merged commit e380459 into colyseus:master Oct 28, 2018
@endel
Copy link
Member

endel commented Oct 28, 2018

Thanks for your pull-request @amit-gshe! It does make sense!
And thanks a lot for taking your time to review this @seiyria ❤️

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

Successfully merging this pull request may close these issues.

None yet

3 participants