v7 middleware functions no longer have server context (undocumented breaking change)

[davidmarkclements]

• Used appropriate template for the issue type
• Searched both open and closed issues for duplicates of this issue
• Title adequately and concisely reflects the feature or the bug

Bug Report

Restify Version

v7.1.0
v7.0.0

Node.js Version

v8.11.1

Expected behaviour

If the breaking change is not deliberate:

The this value of a middleware function should be the server instance

If the breaking change is deliberate:

The changelog and migration guide (http://restify.com/docs/6to7/) should document the change

Actual behaviour

The this value is undefined (or global in non-strict context)

Repro case

'use strict'

const restify = require('restify')
const server = restify.createServer()

server.use(function (req, res, next) {
  console.assert(this === server, 'function context is server instance')
  next()
})

server.get('/', function (req, res, next) {
  res.send('hello')
  next()
})

server.listen(3000)

Cause

handler isn't called with server instance in the call function in chain.js (Chain would have to be instantiated with a server instance passed as an option)
https://github.com/restify/node-restify/blob/master/lib/chain.js#L160
https://github.com/restify/node-restify/blob/master/lib/chain.js#L164

Or else self should be bound to handler in the use method:
https://github.com/restify/node-restify/blob/master/lib/server.js#L506

Are you willing and able to fix this?

Yes

[davidmarkclements]

tangentially related to #1595

[hekike]

Thanks for reporting it!
Was it an expected and documented behavior before v7.x? cc @DonutEspresso @retrohacker

[DonutEspresso]

Handlers were bound to the server in older versions, though in retrospect I don't think its ever been "documented" as part of the public API. It appears to have been an unofficial part of the contract, however, as it's come up many times in different issues and pull requests (and acknowledged by us as well).

I think we have two choices here:

1. rebind the server context
2. document breaking change for 7.x

I don't have a strong preference here, but if there no penalty for binding (or maybe it's an option?), let's go for it.

[leodutra]

Time for benchmarking .call() and .apply() I remember .bind() is more costful than these. Em sex, 6 de abr de 2018 21:56, Alex Liu <notifications@github.com> escreveu:

…

Handlers were bound to the server in older versions, though in retrospect I don't think its ever been "documented" as part of the public API. It appears to have been an unofficial part of the contract, however, as it's come up many times in different issues and pull requests (and acknowledged by us as well). I think we have two choices here: 1. rebind the server context 2. document breaking change for 7.x I don't have a strong preference here, but if there no penalty for binding (or maybe it's an option?), let's go for it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1639 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbHuPkMR3xPrnX2aSyytZlnmZrjiM9qks5tmA6vgaJpZM4TGvxw> .

[DonutEspresso]

Yes, absolutely. Sorry, I was using the word bind to more generally mean binding the this context, independent of how we do it.

[davidmarkclements]

@leodutra bind is now very fast (since Turbofan in V8)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed as stale because it has not had recent activity.

[jrnail23]

this is still an issue... cross posted from #1668:

I'm currently upgrading an app that depends on this in route handler chains being the server instance, and it's now undefined since installing v7.1.1.