Shared state causes side effects across modules

[benurb]

Hey guys,

currently there is a major problem with dynamic-config, because you cannot create an instance of the config loader. So when using npm >= 2 and dynamic-config can be deduped, you will end up with modules sharing the same dynamic-config instance. That means they are overriding each others plugin settings.

Example:
module a uses dynamic-config like this:

var dynamicConfig = require("dynamic-config");
dynamicConfig.use(require("dynamic-config/plugins/extend/env"));
module.exports = dynamicConfig(__dirname, "config.js");

module b uses dynamic-config like this:

var dynamicConfig = require("dynamic-config");
module.exports = dynamicConfig(__dirname, "config.js");

Now it depends on the order:
If module a is loaded before module b you will be able to overwrite configs of both modules via environment variables. This is not intended.
If module b is loaded before module a you will only be able to overwrite configs of module a. This is the intended effect.

This problem is even worse with the file extend plugin:
Module a:

var dynamicConfig = require("dynamic-config");
dynamicConfig.use(require("dynamic-config/plugins/extend/file"));
module.exports = dynamicConfig(__dirname, "config.js");

Module b:

var dynamicConfig = require("dynamic-config");
dynamicConfig.use(require("dynamic-config/plugins/extend/file"), {
  suffix: ".test"
});
module.exports = dynamicConfig(__dirname, "config.js");

If module a is loaded before module b dynamic-config will look for a config.local.js file in both modules, because .local is the default suffix. This is not correct behavior.
If module b is loaded before module a dynamic-config will look for a config.test.js file in both modules. This is also not correct. It seems plugins can only be registered once.

In my opinion it should be possible to create an instance of dynamic-config to be able to maintain the plugin configuration the developer wanted to use. I can create a PR, but wanted to ask for your opinion first 😄

Ben

Update:
Possible code example for the proposed change:

var dynamicConfig = require("dynamic-config").createInstance();

This can probably be implemented as a backwards compatible change.

Of course something like this would be fine, too, but is a breaking change ofc:

var dynamicConfig = new (require("dynamic-config"))();

[meaku]

Thanks @benurb for pointing out. The npm change has a lot of side effects nobody was thinking about in the first place :)

If var dynamicConfig = require("dynamic-config").createInstance(); works without breaking changes, then it's the way to go imho. Do you propose returning an instance (for backwards compatibility) with a createInstance() method to solve the current hazzle?

[benurb]

Yes, it's basically a factory pattern.
Nevertheless I would prefer the breaking change. I cannot imagine a case where the current behavior is desired. It's totally unpredictable, because it depends on loading order. But of course the factory method would solve the problem, too. What's your opinion?

[jhnns]

I'm always surprised that the shift from npm2 to npm3 works so well for most projects, because imho many modules suffer from this issue.

In this particular case, I'm a little bit surprised that you have two modules which depend on dynamic-config. Shouldn't dynamic-config be a typical module that is only required by the actual application? Why is your dependency using dynamic-config?

Besides that I agree with @benurb. This should be a breaking change. Existing applications should be very easy to upgrade.

[benurb]

Actually already npm2 is an issue here, because it dedupes after install.

My specific use case is that we have a project, which runs integration tests on our backend infrastructure. Therefore it lists all the services as dependencies and each of these services uses dynamic-config. Pre-npm2 each service would have a node_modules/serviceX/node_modules/dynamic-config module, but since npm2 they all share the same instance in node_modules/dynamic-config, because the "outer" module also has a dynamic-config dependency.

[jhnns]

In your particular case, the argv and env plugin don't really make sense because they assume a 1:1 relationship between dynamic-config instance and process (or otherwise equally named config keys will clash).

So imho in your situation it's more suitable to spin up multiple processes as it reflects the actual use-case better.

Your proposed change, however, is still reasonable because you might want to use dynamic-config for multiple configs. In this case, however, still the argv and env plugin don't really make sense.

[jhnns]

Fix shipped as breaking change with 1.0.0