Make clock stubbable from consuming apps

[hurrymaplelad]

Currently, this module depends on node-clock. It may or may not share it's clock instance with consuming apps.

When this module installs its own clock instance, clock.now() will not be stubbed in consuming app specs.

Options include:

1. Enforce clock singleton using globals, a la node-fibers
2. Pass the clock into a module config method, a la goodeggs-emailer.
3. Don't call clock.now() within formatters, instead pass in base time to methods that do relative time formatting.

I lean towards option 3, but it's the biggest interface change.

[bobzoller]

seems like even if you go with #3, #1 should also be done. I opened goodeggs/goodeggs-clock#3

On Thu, Mar 19, 2015 at 10:03 AM, Adam Hull notifications@github.com wrote:

Currently, this module depends on node-clock. It may or may not share it's clock instance http://bites.goodeggs.com/posts/commonjs-modules-make-brittle-singletons/ with consuming apps.

When this module installs its own clock instance, clock.now() will not be stubbed in consuming app specs https://github.com/goodeggs/garbanzo/blob/8b97f627aa8600a4b6668f665ae5192e083637b6/src/nettle/spec/server/lib/emailer.spec.coffee#L394 .

Options include:

1. Enforce clock singleton using globals, a la node-fibers https://github.com/laverdet/node-fibers/commit/d9bc3a7b9d486d6f45170501de8626d52dfa5dfa
2. Pass the clock into a module config method, a la goodeggs-emailer https://github.com/goodeggs/goodeggs-emailer.
3. Don't call clock.now() within formatters, instead pass in base time to methods that do relative time formatting.

I lean towards option 3, but it's the biggest interface change.

— Reply to this email directly or view it on GitHub https://github.com/goodeggs/goodeggs-formatters/issues/2.

[asalant]

@bobzoller I'm not sure I agree. The only reason I can see that you need a single instance of clock app-wise is for the stubbing use case in tests. I could see there being benefits to being able to have different versions of clock loaded by modules.

Another solution is for goodeggs-formatters to just have node-clock as a development dependency, essentially requiring that the parent app has installed node-clock. This is basically a peer dependency strategy without using peer dependencies.

[TheBigSadowski]

@asalant there was a small discussion about this and we ( @hurrymaplelad @dlau and I ) came to the conclusion that 2 modules sharing data through a shared dependency was a rather fragile approach. You run the risk of them starting to require different versions and losing the shared state. We felt it was best to be explicit about the sharing and therefore prefer either option 1 or option 3. Having both available would probably be the most flexible.

[asalant]

@TheBigSadowski in this case I'm not sure I see the issue you describe - "You run the risk of them starting to require different versions and losing the shared state." I'm proposing that goodeggs-formatters would explicitly use its parent's version. You wouldn't start using it's own version again.

I'm reacting primarily to the use of a global as a solution. The design of npm is explicitly to allow for non-singleton versions of modules to be used. node-clock is stateless so the only shared state we are talking about is the stubbing of clock.now in parent app tests. Having node-clock go to a global singleton to address that feels like a very heavy hammer solution.

I generally agree with the community that global's should be avoided if there is an alternate design to solve the problem at hand. I do understand there are cases when it is a necessary/appropriate solution (node-fibers makes sense to me).

[TheBigSadowski]

ya. I think I was thinking something more like this could be done...

clock = require 'node-clock'
format = require('goodeggs-formatters).create(clock)

format.date ...

I'm not a fan of the globals either.

[asalant]

@TheBigSadowski got it. That's option 2 (not 1 or 3).