Skip to content

Cleanup middleware config opts - #796

Merged
bajtos merged 2 commits into
masterfrom
feature/cleanup-middleware-config-opts
Nov 18, 2014
Merged

Cleanup middleware config opts#796
bajtos merged 2 commits into
masterfrom
feature/cleanup-middleware-config-opts

Conversation

@bajtos

Copy link
Copy Markdown
Member

Rename config.config to config.params, as that better describes the purpose of the property, which is to pass arguments to the middleware factory function.

Improve jsdoc comments.

/to @ritch please review
/cc @raymondfeng

@bajtos

Copy link
Copy Markdown
MemberAuthor

Note: lib/server-app.js is not included in the apidocs yet, see strongloop/strong-docs#57

@bajtos

Copy link
Copy Markdown
MemberAuthor

@ritch args is weird... It hints at a function call which is a bit odd in a config file. Although I'm not 100% against it since we get the same behavior.

While most middleware factories accepts a single "options" argument, there are exceptions like morgan(format, options) that need multiple arguments.

@raymondfeng suggested options in a comment elsewhere. Initially I thought it is a good candidate, but now that I look at morgan(format, options) as described in morgan docs, I think it would be confusing for users.

Let's explore few alternatives.

{
"morgan-1": {
"args": ["dev", { "immediate": true }]
},
"morgan-2": {
"params": ["dev", { "immediate": true }]
},
"morgan-3": {
"config": [ "dev", { "immediate": true }]
}
}

Perhaps params is a good name, since it can be interpreted both as a list of arguments (params: [1, 2]) and an options object (params: { foo: 1, bar: 2 }).

{
"morgan": {
"params": ["dev", { "immediate": true }]
},
"compression": {
"params": { "threshold": 128 }
}
}

Thoughts?

Miroslav Bajtoš added 2 commits November 14, 2014 09:52
 - Rename `config.config` to `config.params`
- Modify methods to return `this` (fluent API)
@bajtos
bajtosforce-pushed the feature/cleanup-middleware-config-opts branch from 0193a25 to 330292cCompareNovember 14, 2014 08:53
@bajtos

Copy link
Copy Markdown
MemberAuthor

I have reworked the patch to use params instead of args. If we agree that params is a good name, then this PR is ready for final review and merge.

@bajtosbajtos mentioned this pull request Nov 14, 2014
8 tasks
@bajtos

Copy link
Copy Markdown
MemberAuthor

@ritch ping

@ritch

Copy link
Copy Markdown
Member

👍

@bajtosbajtos assigned bajtos and unassigned ritchNov 18, 2014
bajtos added a commit that referenced this pull request Nov 18, 2014
@bajtos
bajtos merged commit 7581ccf into masterNov 18, 2014
@bajtos
bajtos deleted the feature/cleanup-middleware-config-opts branch November 18, 2014 18:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@bajtos@ritch@crandmck