The road to 1.0.0

[Zertz]

@florianholzapfel I've noticed a few TODO's in your code:

1. https://github.com/florianholzapfel/express-restify-mongoose/blob/1.0.0-dev/lib/express-restify-mongoose.js#L663
2. https://github.com/florianholzapfel/express-restify-mongoose/blob/1.0.0-dev/lib/express-restify-mongoose.js#L715

These two are related to options.strict which states:

When set to true, disallows DELETE all, POST with id param, and PUT without id param

• DELETE all should be removed, unless there really is use-case where removing every single document from an API is both useful and safe
• POST with id param is fine and possibly shouldn't even be flagged behind strict
• PUT without id param doesn't make sense since PUT is meant for known resources

What I'm getting to is: is options.strict even necessary?

1. https://github.com/florianholzapfel/express-restify-mongoose/blob/1.0.0-dev/lib/express-restify-mongoose.js#L262
2. https://github.com/florianholzapfel/express-restify-mongoose/blob/1.0.0-dev/lib/express-restify-mongoose.js#L327

I haven't really looked deep into these two. The former could be deprecated, but removing it entirely would be a huge breaking change I believe. The latter mostly looks like a refactor/feature, which can wait.

Also, it would be useful if the req object was passed to outputFn.

Finally, I've gone through issues and there doesn't seem to be anything in there that would introduce more breaking changes. Could you confirm?

@syzer I'll tackle #72 sometimes in the future, I think it's a useful feature and definitely one I'd use. As for PR #56, feel free to chime in!

Other than that, quite a few issues (#112, #109, #81, #78, #52 and #51) seem to be at least vaguely related, but non-breaking.

[florianholzapfel]

I think it is ok to remove the strict option and you are right, PUT does not make sense without an ID.

But I would like to keep the DELETE: Although it does not make much sense to remove entire collections using DELETE, you can include query parameters in the DELETE call to bulk delete documents that are matching your query.

Removing the code mentioned in https://github.com/florianholzapfel/express-restify-mongoose/blob/1.0.0-dev/lib/express-restify-mongoose.js#L262 would be a breaking change for existing users. I would like to keep this one.

[syzer]

strict might be default.
BTW 👍 good work

[Zertz]

You're obviously right about delete, for some reason I completely forgot about passing query parameters. I'll take care of removing strict today.

I agree, L262 would break client apps in a major way so it must stay.

What do you think about outputFn? I've come across cases where having access to the req object would be handy. This would be breaking obviously, but it's a minor change for users.

I was thinking maybe it could actually be middleware or close to it:

(req, res, next, result)

or

(req, res, next, {
  result: result,
  status: status
})

Something like that. Ideas?

[florianholzapfel]

Yes, having the original request available in the output function can really be useful. I would prefer the second variant, since it provides a status.

[Zertz]

Oh, I forgot to add status to the first, but an object seems better suited anyway as it leaves the door open to pass more parameters.

I'm not sure if next is useful at all, but I guess going with a standard signature is a good direction to head in and someone might stumble on a use-case for it.

[Zertz]

@florianholzapfel One more thing. The async prereq callback should be cb(err, allow) rather than cb(allow). Currently, if an error happens the best we can do is return 403 which isn't really what happened.

[syzer]

also im using promisification
and it works for cb(err, res) good
or cb(null, res)

but for cb(res) not that great

On Thu, May 7, 2015 at 3:52 PM, Pier-Luc Gendreau [email protected]
wrote:

@florianholzapfel https://github.com/florianholzapfel One more thing.
The async prereq callback should be cb(err, allow) rather than cb(allow).
Currently, if an error happens the best we can do is return 403 which isn't
really what happened.

—
Reply to this email directly or view it on GitHub
#115 (comment)
.

--Lukas 'SyZer' Gintowt

[Zertz]

@florianholzapfel Sorry for the annoyance once more! I'd like your input on this one. I hadn't realized, but there are repercussions to passing next over to outputFn.

The situation arises at a few places, including there: https://github.com/florianholzapfel/express-restify-mongoose/blob/master/lib/express-restify-mongoose.js#L465

The way I see it, either we pass next and remove the postProcess/Create/Delete methods or we only pass (req, res,{ result: result, statusCode: statusCode })

I like the former, but the latter is definitely more reasonable and less breaking.

[syzer]

next() allows using other middleware
i'm in favour of next()

On Thu, May 7, 2015 at 5:49 PM, Pier-Luc Gendreau [email protected]
wrote:

@florianholzapfel https://github.com/florianholzapfel Sorry for the
annoyance once more! I'd like your input on this one. I hadn't realized,
but there are repercussions to passing next over to outputFn.

The situation arises at a few places, including there:
https://github.com/florianholzapfel/express-restify-mongoose/blob/master/lib/express-restify-mongoose.js#L465

The way I see it, either we pass next and remove the
postProcess/Create/Delete methods or we only pass (req, res,{ result:
result, statusCode: statusCode })

I like the former, but the latter is definitely more reasonable and less
breaking.

—
Reply to this email directly or view it on GitHub
#115 (comment)
.

--Lukas 'SyZer' Gintowt

[Zertz]

I agree, however, (and I could be wrong here) the problem with passing next is the weird behavior that could crop up with the existing post* methods. next would be called by restify right after calling outputFn and then potentially once again by the user's custom outputFn.

So if we do that, I'm pretty sure it would be necessary to remove the post* methods.

I'm going to send the PR without next for now while this is being figured out.