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

Make the client argument optional #24

Merged

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented Dec 4, 2015

According to the documentation the constructor takes two optional arguments, but calling the constructor like this:

var client = new SteamUser({ dataDirectory: __dirname + '/data' });

returns an error.
This patch should fix the issue.

@DoctorMcKay
Copy link
Owner

This won't work if the app imports Steam from somewhere different from where SteamUser gets it.

For example:

 | app.js
 | node_modules
   |----steam
   |----steam-user
        |----node_modules
             |----steam

app.js does require('steam') and instantiates a SteamClient from there. That SteamClient won't be an instanceof SteamUser's SteamClient.

@lpinca
Copy link
Contributor Author

lpinca commented Dec 5, 2015

Good point, what about checking if it's an EventEmitter?

@DoctorMcKay
Copy link
Owner

That sounds reasonable.

@lpinca
Copy link
Contributor Author

lpinca commented Dec 5, 2015

Or checking if the constructor name is SteamClient.

foo.constructor.name === 'SteamClient';

@DoctorMcKay
Copy link
Owner

That sounds even better.

@lpinca lpinca force-pushed the fix/constructor-optional-arguments branch from 623dbbc to 83968c1 Compare December 5, 2015 07:20
@lpinca
Copy link
Contributor Author

lpinca commented Dec 5, 2015

Updated.

DoctorMcKay added a commit that referenced this pull request Dec 5, 2015
@DoctorMcKay DoctorMcKay merged commit 5073676 into DoctorMcKay:master Dec 5, 2015
@DoctorMcKay
Copy link
Owner

Thanks.

@lpinca lpinca deleted the fix/constructor-optional-arguments branch December 5, 2015 07:33
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

2 participants