Move config into a local singleton #7

tswicegood · 2017-02-17T17:20:43Z

Why are we doing this?

I'm trying to extract all of the shared state into something that can be accessed across modules. Right now the this.config inside Messenger is the majority of the shared state. In talking this through with @stripethree, we realized there might be an issue with non-namespaced configuration as well. This introduces launch-vehicle-fbm. as the prefix for all configuration variables.

Did you document your work?

Haven't yet – there currently isn't any configuration in the README.md so nothing's changed.

How can someone test these changes?

npm i
npm t
No test failures and three new tests under the config module

What possible risks or adverse effects are there?

None, assuming the test coverage has caught edge cases for configuration changes. A few tests failed with the change, but that was caused by the constructor changing.

stripethree

In our IRL conversation we talked about having an application level configuration that we recommend developers use. Is that something we are interested in articulating in the README? That also seems like it would be useful to have a simple, supporting getter function as well:

getAppConfig() {
  return config.has("application") ? config.get("application") : {};
}

I don't view this as required, and it could also be a follow-up to this PR; to me this serves to solidify the delineation between SDK config and application config.

stripethree · 2017-02-17T17:26:14Z

src/app.js

@@ -360,15 +358,15 @@ class Messenger extends EventEmitter {
    const signature = req.headers['x-hub-signature'];

    if (!signature) {
-      throw new Error(`Couldn't validate the signature with app secret: ${this.config.get('messenger.appSecret')}`);
+      throw new Error(`Couldn't validate the signature with app secret: ${config.get('messenger.appSecret')}`);


Something nags at me about us having the appSecret in the error message here, but that's also existing and was not added in this PR.

Agreed. Feels like this might be a good place for custom Error subclass that provides the additional context outside of the message property. Not sure how / if Error subclasses works as expected in JavaScript.

pgoldrbx · 2017-02-17T17:35:06Z

+1 for namespace

Q: would this change mean that downstream users of the SDK wouldn't be able to override or extend the app config?

stripethree · 2017-02-17T17:33:12Z

src/config.js

+    key = `${prefix}${key}`;
+  }
+  return config.get(key);
+};


Might also want to warp has, as that could be used in conversationLogger. I think it'd look like this:

exports.has = function (key, prefix = 'launch-vehicle-fbm') { if (prefix) { key = `${prefix}${key}`; } return config.has(key); };

@stripethree agreed – let's add that in when we've got the use-case though.

stripethree · 2017-02-17T17:36:30Z

src/config.js

+
+exports.config = config;
+
+exports.get = function (key, prefix = 'launch-vehicle-fbm.') {


NITPICK: In the mindset of a possible contributor, I would think the separator would be handled by this wrapper function, not as part of the prefix argument. The exception would be if someone else were to swap an alternate package in place of node-config, but I think that would take more changes across the SDK.

config.get('modiface', 'apiUrl')

if (prefix) { key = `${prefix}.${key}`; }

Extracted this into getNamespaceKey so it can be tested independently and can be reused in a has function down the road.

stripethree · 2017-02-17T17:48:15Z

@pgoldrbx - Extend, yes. We talked about having an application namespace at the same level that we recommend for use. That's what this comment is referring to. Override... not without changing the SDK to modify our specific prefix.

Extending will be a given because there are things built in that are optional, like Dashbot, that we would probably prefer the user to specify within the SDK namespace. I think we should follow this PR up with moving the conversationLogger to use the config object instead of pulling directly from the env.

tswicegood · 2017-02-17T17:56:15Z

@stripethree sorry – I totally took something different away. I was thinking that we would namespace all of our configuration then let the end-user do whatever they like so long as they realize that everything in launch-vehicle-fbm was owned by us.

stripethree · 2017-02-17T19:50:34Z

@tswicegood That's fine too! I sometimes end up favoring or wanting more structure than is really needed (and may not have articulated that well in our convo). I have at least one personal project I want to use with this once we open it up and might experiment with some config things there.

crccheck · 2017-02-17T20:08:10Z

I was actually thinking we should get rid of the dependency on config and have everything become an option in the constructor. I never got around to it when it was sharing with the app, but the only real reason why it's not like that now is because it's a really fat object to pass in, but very typical.

tswicegood · 2017-02-17T21:11:41Z

@crccheck if it's cool with you, I'd like to merge this and we can tackle possibly removing configuration all together. I think there's some value in that for sure, but that seems tangential to what what I need in #6 (a way to break this out of the state of Messenger).

crccheck · 2017-02-17T21:23:41Z

What about this...

Original:

this.config.get('messenger.appSecret')

This PR:

const config = require('./config')
config.get('messenger.appSecret')

My proposal:

const config = require('config')
config.get('launch-vehicle-fbm.messenger.appSecret')

You get the same config singleton, you get the namespacing, you get the standard usage of the config package. But you just got these long strings you have to deal with. You could do ES6 template strings (${PREFIX}messenger.appSecret) but I think since it doesn't really save on line length, we say use find/replace, it exists.

In other words: I'm for using config the way you're supposed to use it. I'm very opposed to writing wrappers.

tswicegood · 2017-02-17T21:36:42Z

@crccheck what's the goal with your proposal? That does save adding an extra module, but increases the duplication of a code by 16x for the current configuration usage. The additional characters that have to be duplicated end up within 5 bytes the whole file size of src/config though, so the savings goes negative as with next config value added.

stripethree · 2017-02-17T21:40:41Z

Why not just assume the namespace is there in the first place?

   const config = require('config').get('launch-vehicle-fbm');
   config.get('messenger.appSecret')

The namespace doesn't really need to be explicit in this codebase beyond that, does it?

Edit: ran a local test and this worked as expected

const config = require('config').get('launch-vehicle-fbm');
console.log(Object.keys(config));
console.log(config.get('facebook.appId'));

[ 'facebook', 'messenger' ]
************6968

tswicegood · 2017-02-17T23:11:10Z

I like that @stripethree. Didn't realize that node-config returned a config object when returning part of the nested structure. It does make testing a little more difficult as the configuration is now at the module level. Given that this is needed in a couple of places (not currently, but will in #6) I still think abstracting this into ./src/config.js is the right answer, but it's drastically simpler.

crccheck

Like! There's only one structural bit I'm not a fan of, and it's such a trivial change to go from require('./config') to require('config').get('launch-vehicle-fbm') that it's not worth fussing over.

like:

namespace config
the config grabbing looks more like how others use the package
the constructor now only takes in one options object as people would expect

dislike:

people are going to wonder why the config layer of abstraction exists

crccheck · 2017-02-17T23:27:18Z

test/config.spec.js

+describe('config', () => {
+  it('should be an instance of node-config with launch-vehicle-fbm', () => {
+    // sanity check
+    expect(nodeConfig.get('launch-vehicle-fbm')).to.not.be.empty;


The other tests use assert instead of chai. I think assert is easier to read and write, but I like chai's error handling. And since tests rarely fail but are read all the time, I stick to assert.

crccheck · 2017-02-17T23:29:13Z

test/config.spec.js

+    // sanity check
+    expect(nodeConfig.get('launch-vehicle-fbm')).to.not.be.empty;
+
+    // actual test


i don't think we need this comment

stripethree · 2017-02-21T19:33:00Z

@tswicegood any outstanding things before squash and merge?

tswicegood added 2 commits February 17, 2017 11:07

Add a config module that is namespaced

f013b41

Switch to new internal config module

44904f9

tswicegood requested review from crccheck and stripethree February 17, 2017 17:20

stripethree reviewed Feb 17, 2017

View reviewed changes

tswicegood added 2 commits February 17, 2017 11:52

Refactor how prefixes are added (idea via @stripethree)

46ad90d

Switch to ternary operator

cfca369

Swap this around to a simpler version based on @stripthree's feedback

4676df2

crccheck approved these changes Feb 17, 2017

View reviewed changes

stripethree approved these changes Feb 17, 2017

View reviewed changes

stripethree merged commit 9908134 into master Feb 22, 2017

crccheck deleted the refactor/config branch February 22, 2017 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move config into a local singleton #7

Move config into a local singleton #7

tswicegood commented Feb 17, 2017

stripethree left a comment •

edited

Loading

stripethree Feb 17, 2017

tswicegood Feb 17, 2017

pgoldrbx commented Feb 17, 2017

stripethree Feb 17, 2017 •

edited

Loading

tswicegood Feb 17, 2017

stripethree Feb 17, 2017 •

edited

Loading

tswicegood Feb 17, 2017

stripethree commented Feb 17, 2017 •

edited

Loading

tswicegood commented Feb 17, 2017

stripethree commented Feb 17, 2017

crccheck commented Feb 17, 2017

tswicegood commented Feb 17, 2017

crccheck commented Feb 17, 2017 •

edited

Loading

tswicegood commented Feb 17, 2017

stripethree commented Feb 17, 2017 •

edited

Loading

tswicegood commented Feb 17, 2017

crccheck left a comment

crccheck Feb 17, 2017

crccheck Feb 17, 2017

stripethree commented Feb 21, 2017


		exports.config = config;

		exports.get = function (key, prefix = 'launch-vehicle-fbm.') {

Move config into a local singleton #7

Move config into a local singleton #7

Conversation

tswicegood commented Feb 17, 2017

Why are we doing this?

Did you document your work?

How can someone test these changes?

What possible risks or adverse effects are there?

stripethree left a comment • edited Loading

Choose a reason for hiding this comment

stripethree Feb 17, 2017

Choose a reason for hiding this comment

tswicegood Feb 17, 2017

Choose a reason for hiding this comment

pgoldrbx commented Feb 17, 2017

stripethree Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

tswicegood Feb 17, 2017

Choose a reason for hiding this comment

stripethree Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

tswicegood Feb 17, 2017

Choose a reason for hiding this comment

stripethree commented Feb 17, 2017 • edited Loading

tswicegood commented Feb 17, 2017

stripethree commented Feb 17, 2017

crccheck commented Feb 17, 2017

tswicegood commented Feb 17, 2017

crccheck commented Feb 17, 2017 • edited Loading

tswicegood commented Feb 17, 2017

stripethree commented Feb 17, 2017 • edited Loading

tswicegood commented Feb 17, 2017

crccheck left a comment

Choose a reason for hiding this comment

crccheck Feb 17, 2017

Choose a reason for hiding this comment

crccheck Feb 17, 2017

Choose a reason for hiding this comment

stripethree commented Feb 21, 2017

stripethree left a comment •

edited

Loading

stripethree Feb 17, 2017 •

edited

Loading

stripethree Feb 17, 2017 •

edited

Loading

stripethree commented Feb 17, 2017 •

edited

Loading

crccheck commented Feb 17, 2017 •

edited

Loading

stripethree commented Feb 17, 2017 •

edited

Loading