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

Update botmaster.js #2

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jwickens
Contributor

jwickens commented Oct 27, 2016

incorrect check for socketio.server leading to overwriting with an undefined value (for some reason this.server is undefined)

Update botmaster.js
incorrect check for socketio.server leading to overwriting with an undefined value (for some reason this.server is undefined)
@jdwuarin

This comment has been minimized.

Show comment
Hide comment
@jdwuarin

jdwuarin Oct 28, 2016

This looks really weird. Not sure this fix would do. I'll investigate tomorrow. For the record, could you send code here of how you're instantiating the botmaster object?

jdwuarin commented on 5475dd8 Oct 28, 2016

This looks really weird. Not sure this fix would do. I'll investigate tomorrow. For the record, could you send code here of how you're instantiating the botmaster object?

@jwickens

This comment has been minimized.

Show comment
Hide comment
@jwickens

jwickens Oct 28, 2016

Contributor

I'm passing in the app:

    const botmasterSettings = {
        botsSettings: botsSettings,
        port: environment.port,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

So that plus the comment on line 38 of botmaster.js tells us why this.server is undefined.

So what happens currently is:

  1. settings.socketio.server exists
  2. there is no settings.server (app only provided)
  3. the check for settings.server replaces settings.socketio.server with the undefined settings.server

So with the current code as is I would have to set server against both settings.server and settings.socketio.server

With the proposed change I can just set the settings.socketio.server. Or I can just set the settings.server.

Contributor

jwickens commented Oct 28, 2016

I'm passing in the app:

    const botmasterSettings = {
        botsSettings: botsSettings,
        port: environment.port,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

So that plus the comment on line 38 of botmaster.js tells us why this.server is undefined.

So what happens currently is:

  1. settings.socketio.server exists
  2. there is no settings.server (app only provided)
  3. the check for settings.server replaces settings.socketio.server with the undefined settings.server

So with the current code as is I would have to set server against both settings.server and settings.socketio.server

With the proposed change I can just set the settings.socketio.server. Or I can just set the settings.server.

@jdwuarin

This comment has been minimized.

Show comment
Hide comment
@jdwuarin

jdwuarin Oct 28, 2016

Member

I think I might see where the issue comes from. But the fix will probably have to be a bit different. I am assuming you have your server object within your socketio settings object am I correct?

For clarity, could you post here what your botsSettings object looks like (and expand on what the socketioSettings looks like too.

The issue here I think is that the code assumes (but still throws an error) that if you are passing in an app object, you would pass in the main settings a server parameter too. I.e. that it would look something like this:

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app,
        server: server,
    };

But really, the server object could very well be within the socketioSettings directly. Which would mean that one would have something like this:

socketioSettings = {
  id: 'some_random_string_of_your_choosing',
  server: server,
}


  const botsSettings = [{ socketio: socketioSettings },...];

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

But currently, (and I am assuming this is what you're doing, this doesn't really work). Only the following works today:

socketioSettings = {
  id: 'some_random_string_of_your_choosing',
}


  const botsSettings = [{ socketio: socketioSettings },...];

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app,
        server: server,
    };
    const botmaster = new Botmaster(botmasterSettings);

I.e. just moving the server object to the botmasterSettings. In order to support both types, the following fix would have to be implemented I believe:

        if (!settings.server && !settings.socketio.server) {
          settings.socketio.server = this.server;
        }
Member

jdwuarin commented Oct 28, 2016

I think I might see where the issue comes from. But the fix will probably have to be a bit different. I am assuming you have your server object within your socketio settings object am I correct?

For clarity, could you post here what your botsSettings object looks like (and expand on what the socketioSettings looks like too.

The issue here I think is that the code assumes (but still throws an error) that if you are passing in an app object, you would pass in the main settings a server parameter too. I.e. that it would look something like this:

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app,
        server: server,
    };

But really, the server object could very well be within the socketioSettings directly. Which would mean that one would have something like this:

socketioSettings = {
  id: 'some_random_string_of_your_choosing',
  server: server,
}


  const botsSettings = [{ socketio: socketioSettings },...];

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

But currently, (and I am assuming this is what you're doing, this doesn't really work). Only the following works today:

socketioSettings = {
  id: 'some_random_string_of_your_choosing',
}


  const botsSettings = [{ socketio: socketioSettings },...];

    const botmasterSettings = {
        botsSettings: botsSettings,
        app: app,
        server: server,
    };
    const botmaster = new Botmaster(botmasterSettings);

I.e. just moving the server object to the botmasterSettings. In order to support both types, the following fix would have to be implemented I believe:

        if (!settings.server && !settings.socketio.server) {
          settings.socketio.server = this.server;
        }
@jdwuarin

This comment has been minimized.

Show comment
Hide comment
@jdwuarin

jdwuarin Oct 28, 2016

Member

Commit: d79b6b5 Fixes this issue and version 2.1.1 has been published with the fix.

Member

jdwuarin commented Oct 28, 2016

Commit: d79b6b5 Fixes this issue and version 2.1.1 has been published with the fix.

@jdwuarin jdwuarin closed this Oct 28, 2016

@jdwuarin

This comment has been minimized.

Show comment
Hide comment
@jdwuarin

jdwuarin Oct 28, 2016

Member

Also, I notice that you're using botmaster's constructor like this:

    const botmasterSettings = {
        botsSettings: botsSettings,
        port: environment.port,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

If you declare an app in your settings, the port will not be used. Because it is assuemed that you are handling everything relating to your express server. So you can just remove the mention to the port in your settings.

Member

jdwuarin commented Oct 28, 2016

Also, I notice that you're using botmaster's constructor like this:

    const botmasterSettings = {
        botsSettings: botsSettings,
        port: environment.port,
        app: app
    };
    const botmaster = new Botmaster(botmasterSettings);

If you declare an app in your settings, the port will not be used. Because it is assuemed that you are handling everything relating to your express server. So you can just remove the mention to the port in your settings.

@jwickens jwickens deleted the jwickens:patch-1 branch Oct 31, 2016

@jwickens

This comment has been minimized.

Show comment
Hide comment
@jwickens

jwickens Oct 31, 2016

Contributor

Well unfortunately I don't have the code where I was calling setting up botmaster with socketio... the project i was using on had some other issues that required me to not do that. Eyeballing the fix, it looks good. Thanks too for the hint about the port setting.

Contributor

jwickens commented Oct 31, 2016

Well unfortunately I don't have the code where I was calling setting up botmaster with socketio... the project i was using on had some other issues that required me to not do that. Eyeballing the fix, it looks good. Thanks too for the hint about the port setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment