koopjs / koop

Transform, query, and download geospatial data on the web.
http://koopjs.github.io
Other
651 stars 125 forks source link

Allow provider controllers to access koop instance #225

Closed timwis closed 8 years ago

timwis commented 8 years ago

Branching off from #195 for something more specific - how should provider controllers access the koop instance? For instance, if they need to access koop.config or plugins or anything else set in koop/index.js. For #195, I just included a reference to the koop instance in the provider's model, which the controller already has access to. Happy to do it another way if you prefer.

Let me know if you'd like a pull request on one of the providers; I just figured since it'd probably need to be done for every provider, a discussion thread would be better.

Also, if the use case here is strictly #195, feel free to merge this discussion back into that thread. I'm assuming there'd be other reasons to access the koop instance.

dmfenton commented 8 years ago

Provider controllers shouldn't access Koop directly, instead they should go through the base controller or the model.

timwis commented 8 years ago

Hm, I don't see where the base controller is being passed the koop instance. Is it?

Also, while the model is passed koop, it doesn't store it anywhere so that a provider controller could reference it, and it looks like BaseModel doesn't either. So there's no way for the controller to access model.koop.

Good point about the base controller/model though - this way you wouldn't have to change every provider. So a pull request would just store the koop instance reference as a property on either or both of the base classes.

dmfenton commented 8 years ago

Here's an example of using koop.config inside a provider model: https://github.com/koopjs/koop-socrata/blob/master/models/Socrata.js#L15-L22

Base controller isn't passed the koop instance, but it does talk to the base model which has access to koop.

ungoldman commented 8 years ago

Ideally none should have a copy of the koop instance. We should be passing config and using an EventEmitter pattern

timwis commented 8 years ago

Okay, so how would I access a plugin from a provider controller? With the current code, I don't believe it's possible. Forgive me if I'm misunderstanding you.

ungoldman commented 8 years ago

Sorry @timwis for my unhelpful response. I was thinking in terms of future architecture again.

I'm not sure there's a precedent for a plugin being accessed from a provider controller -- the only plugin to date is https://github.com/koopjs/koop-tile-plugin. @dmfenton do you have an idea of how this might work best with the current setup? Currently I think @timwis is right in that we would need to pass the entire koop object into the controller constructor to allow the provider access to the plugin.

dmfenton commented 8 years ago

@ngoldman @timwis I talked with @chelm on this and we agreed that the best/least worst way is to modify the base model to use a plugin if it's available. koop-tile-plugin is used by koop-agol:

agol.tileGet calls tileGet in Koop's base model.

Plugins should definitely be more loosely coupled, but this is the way for now.

ungoldman commented 8 years ago

Yikes! That's not very plugin-y. I'll work on improving that in the near future. Hope that helps in the interim @timwis.

timwis commented 8 years ago

Well, this would be for both koop-odata and soda2 interfaces, so just to be clear you're talking about adding two plugin-specific methods to the BaseModel

dmfenton commented 8 years ago

unfortunately, yes

timwis commented 8 years ago

Check out pull #226 - implemented basically the same way but a little more pluginy and less hardcodingy. You'd access it via ckan.plugin('odata') What do you think?

ungoldman commented 8 years ago

@timwis makes sense for now, I'll work on a more ideal pattern for a future major version (most likely koop v3).