krakenjs / express-enrouten

An express route initialization and configuration module.
Other
172 stars 38 forks source link

Add a way to pass configuration into handler setup controllers #62

Open aredridel opened 9 years ago

aredridel commented 9 years ago

As a vague example, maybe something like accepting functions in the form of

module.exports = function (router, config) {
}

This would solve one of the most common questions I get in the context of Kraken: "How do I handle configuration? I can't get to it from here"

sixlettervariables commented 9 years ago

+1 (I'd use this feature today)

jasisk commented 9 years ago

So the complexity here is that enrouten has no concept of config. Thinking this over, I think the simplest way to implement this would be to add another internal shortstop handler to confit—similar to the config handler—that would be replaced with a reference to the completely built config factory. That way, it could be passed as an argument (or option) to enrouten.

Just a thought.

aredridel commented 9 years ago

Yeah. Some sort of opaque thing passed in? If it's config, yay! when used by Kraken.

totherik commented 9 years ago

Yep, this is a long time coming but really is a form of DI. As I start contemplating I welcome any thoughts or ideas.

EDIT: And yes, I'll start digging into your suggestion @jasisk.

totherik commented 9 years ago

One thing that just occurred to me is that this will necessarily force a major version change (I was hoping it wouldn't). The current rule for directory mode is that the traversal mechanism will only initialize files that accept a single argument, allowing route definitions to be intermingled with other files. By having user-defined arguments the mechanism for identifying those files becomes much more murky.

aredridel commented 9 years ago

Oh, that is just unfortunate!

totherik commented 9 years ago

It may be a blessing in disguise. Gives us a little more latitude to take advantage of better directory layout and improve config. I've put together a very rough proposal if anyone has feedback.

aredridel commented 9 years ago

Aye. That seems like an awful lot of DI machinery and jargon to drag in, and it means that we're coupled to doing dependency injection our way, which just makes it unpleasant for people who have another opinion there.

aredridel commented 9 years ago

I see the intent, but something is rubbing me really wrong about it.

totherik commented 9 years ago

Yep, re: DI. I was considering using an existing implementation by default and injecting the router itself that way, but I haven't yet found a "lightweight" module, hence the code I put together. If we go through with this I'd prefer to defer to an existing module should we go in whole hog, and use it to manage all dependencies, including router.

aredridel commented 9 years ago

I dunno. It feels a lot like a lot of conceptual overhead to use an otherwise simple module.

totherik commented 9 years ago

I'm im not sure that simplicity and configurability are mutually exclusive. The request to inject arbitrary objects during initialization isn't the basic, simple use-case here. A smart default of always injecting the router will most certainly help.

aredridel commented 9 years ago

Indeed.

grawk commented 9 years ago

regarding the single argument function, just because a function signature has a single argument doesn't mean the function cannot be called with additional arguments

totherik commented 9 years ago

True, but I'm personally not willing to codifyvar myvar = arguments[1]; as a desired pattern. :)

grawk commented 9 years ago

What about attaching the config as a property of the router?

jasisk commented 9 years ago

That's interesting. We already provide a facade there.

totherik commented 9 years ago

Well attaching config explicitly so will create a tight coupling between kraken-js and enrouten, something that I'm trying to avoid. The only way to avoid that would be to support something like an additionalProperties config property that would be an object of arbitrary stuff which would get attached to the router instance.

With a design that supports multiple arguments the default case (just router) could possibly keep the current scanning mechanism, but when additional dependencies are configured to be injected it could switch into a more liberal scanning mode. That would preserve backward compat.

grawk commented 9 years ago

the single argument could be an object but the user would need to be aware of when they have triggered DI in order to pull the router argument apart

pvenkatakrishnan commented 9 years ago

+1 to adding additional params as a config

totherik commented 9 years ago

Does someone want to put together a proposal?

grawk commented 9 years ago

I don't have much time today and tomorrow. But if it's still dangling when I'm free I would like to take a crack at it.