Do not remove unrecognized configuration keys

[matthieusieben]

However not ideal, we do monkey patch our User Agent (e.g. #211, #217). We prefer doing this than to fork since it is easier to stay up-to-date this way.

Some of our patches require some configuration. We also use the configuration object to store some data on the user agent. Again, this might not be ideal but this is the most convenient way of linking data to the user agent without having to manage an external store.

Would it be possible to keep in the configuration object, any configuration item that is not recognized ? If I propose a Pr for this, does it have a chance of going through ?

[matthieusieben]

Current workaround (yet another monkey patch 😆)

var loadConfigOrig = UA.prototype.loadConfig;
UA.prototype.loadConfig = function (configuration) {
  loadConfigOrig.apply(this, arguments);

  for (const key in configuration) {
    if (!(key in this.configuration)) {
      this.configuration[key] = configuration[key];
    }
  }
};

[james-criscuolo]

I believe this is is why the UA (and all objects) have a data object and this is how we internally use the data object. Is that not sufficient for your needs? You'd have to insert your extra information after UA creation, but if you haven't altered any of the rest of the UA creation I think it would work.

James

[matthieusieben]

Both the data object and the configuration object allow indeed to store additional data. This request is more about making it easier to manager custom configuration data.

Consider for example the following snippet (the actual workaround we use for #217). Our configuration object is generated by an API call (this allows us to manage sip peers dynamically or to change default config, such as #389, without requiring the client to update their app).

In this particular case, it is much more convenient to have custom configuration fields, than having some additional code to copy custom configuration to the data object for our custom wsServerConnectingTimeout configuration.

  return new UA(configuration)
    .on('connecting', handleConnecting)
    .on('connected', handleConnected)

  function handleConnecting () {
    if (this.configuration.wsServerConnectingTimeout > 0) {
      clearTimeout(this.transportConnectingTimer)
      this.transportConnectingTimer = setTimeout(() => {
        this.transportConnectingTimer = null
        if (this.transport.ws.readyState !== WebSocket.OPEN) {
          this.logger.warn('WebSocket connection timeout.')
          this.transport.ws.close()
        }
      }, this.configuration.wsServerConnectingTimeout * 1000)
    }
  }

  function handleConnected () {
    clearTimeout(this.transportConnectingTimer)
    this.transportConnectingTimer = null
  }

I figured, since adding custom configuration fields would not hurt the operation of SIP.js core files, why prevent it?

[james-criscuolo]

I'm looking into #388 now, and this seems like something that can be accommodated in the process. I would rather not put these custom variables next to the normal configuration options, so I'm thinking ua.configuration.custom will be their home. Does that sound feasible? The code will pull all of the "standard" config options off of what is passed in, then the remaining object will just be set to custom.

[matthieusieben]

In that case I would rather simply copy the custom key from the input configuration into ua.configuration as is. Having something that moves from configuration.mySetting to ua.configuration.custom.mySetting is less intuitive than if it came from configuration.custom.mySetting in the first place, don't you think?
Plus, this would avoid having to somehow detect in the code which key is "official" and which isn't.

Note that this extra feature is kinda useless to the library (and it is redundant with ua.data). This is why I proposed to use ua.configuration directly: There would be no useless feature in SIP.js, only a hidden behaviour that allows you to use any custom configuration key without removing it. I had to look to the code in order to understand why my custom configuration keys were dropped.

So, to sum up, I prefer having the behaviour I proposed initially as I think it is more intuitive and does not add apparent complexity to the library. Otherwise, I would require users to use ua.data.

As long as I can at least use my monkey patch I'll be happy 😆

[james-criscuolo]

I agree that moving things from configuration.mySetting to configuration.custom.mySetting does sound more confusing than just having configuration.custom.mySetting passed in, so I'll switch that.

My reasons for keeping custom settings separate are to keep intended library functionality isolated from anything people add. I foresee people adding random things to their config, then asking us why it doesn't work, only for us to find that we do not claim support for such options (sorry for the pessimism). SIP.js attempts to prevent "pollution" of its objects via the data properties, but I think adding a custom property has value, as you can just pass your whole config at once. Instead of making changes after the fact. Having the ability to say "leave out your custom config options and try again" is powerful, and this makes that much easier, especially for people unfamiliar with the library.

Given these changes, I'd still expect the monkey patch to work 👍

[matthieusieben]

Alright then 😁

[james-criscuolo]

Just committed these changes (along with the other issue) here: 1245245