koopjs / koop-pgcache

PostGIS cache for Koop.
Other
4 stars 4 forks source link

passing koop as a parameter in `connect` #31

Open ungoldman opened 8 years ago

ungoldman commented 8 years ago

Is sharing the logger the only reason we're passing the entire koop object into the connect method?

https://github.com/koopjs/koop-pgcache/blob/2df3ccc0407859f5f86a91d12e6a7eb6d5a3034d/index.js#L28

If so we should probably just pass the log.

dmfenton commented 8 years ago

I don't think so. I think this says it all: https://github.com/koopjs/koop-pgcache/blob/2df3ccc0407859f5f86a91d12e6a7eb6d5a3034d/index.js#L22

ungoldman commented 8 years ago

Right, that says "an instance of koop, mainlt for central/shared logging", which I think means it's just for sharing the logger instance. If you search for koop in that file it is only being referenced once, right here:

https://github.com/koopjs/koop-pgcache/blob/2df3ccc0407859f5f86a91d12e6a7eb6d5a3034d/index.js#L28

this.log = koop.log

The pgccache module is used directly by the Cache instance on koop as koop.Cache.db, and connected only once here:

https://github.com/koopjs/koop/blob/master/index.js#L137

We could make this relationship more clear by renaming koop to options in the params and specifying that the only option is log. This maintains compatibility while clarifying the function's API. In a future major version we could get rid of the need for that param altogether by emitting log events using the EventEmitter pattern we've discussed before.

ungoldman commented 8 years ago

https://github.com/koopjs/koop/issues/228

dmfenton commented 8 years ago

Another thing to add to the 2.0 release would be just having a logger passed in, instead of an instance of Koop.

ungoldman commented 8 years ago

:+1: yep