Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

res.format(): call default using obj as the context #3587

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

shesek
Copy link
Contributor

@shesek shesek commented Mar 13, 2018

... so that it may refer to the other handlers via this. This is useful for cases where you want to delegate the default behavior to one of the other registered handlers, e.g.:

app.get('/:item', (req, res) => res.format({
  json() { res.send(req.item) }
, text() { res.send(req.item.toString()) }
, xml()  { res.send(req.item.toXML()) }
, default() { this.json() }
}))

Without this, I have to pre-declare the default json handler separately, making it inconsistent with how the non-default handlers are declared and making the overall code more complex:

app.get('/:item', (req, res) => {
  function sendJSON() { res.send(req.item) }
  res.format({
    json: sendJSON
  , text() { res.send(req.item.toString()) }
  , xml()  { res.send(req.item.toXML()) }
  , default: sendJSON
  })
})

The regular (non-default) handlers are already called with the obj as their context, so this would match their behavior too.

@shesek
Copy link
Contributor Author

shesek commented Mar 13, 2018

And a question: is there a reason (req, res, next) aren't sent to the default handler?

@dougwilson
Copy link
Contributor

This is very interesting, thank you! The res.format piece is pretty old and hasn't been visited in a while, and I don't even know the answer to your question since that decision was made before I was involved in the project.

As for the PR, most users are using the => syntax these days and so the this is no longer a useful way to make functionality available to the called function. What do you think about instead passing it as an argument? Also, argument or this, shouldn't all the invocations (even the non-default ones) act the same? If one would want to call this.json() in the default handler, it doesn't seem too far-fetched to think one would want to called this.text() in the html handler in order to wrap the output in HTML.

@shesek
Copy link
Contributor Author

shesek commented Mar 27, 2018

As for the PR, most users are using the => syntax these days and so the this is no longer a useful way to make functionality available to the called function.

You're correct, but the object method syntax (used in the OP) is also quite convenient here (and is even shorter by one character).

What do you think about instead passing it as an argument?

Since the non-default handlers are already receiving 3 arguments, this would have to be the 4th, right?

How about we fn.call(formatObj, req, res, next, formatObj), so that it'll be available both via this and as an argument?

Also, argument or this, shouldn't all the invocations (even the non-default ones) act the same? If one would want to call this.json() in the default handler, it doesn't seem too far-fetched to think one would want to called this.text() in the html handler

With this PR, the default and non-default handlers are both called with the this context set to the format object. The only difference in their invocations is the lack of (req, res, next) which I was pointing out.

@dougwilson
Copy link
Contributor

I know this is old and was forgotten about, but still seems relevant. I updated your PR to add a test case. I think the point about default having a different context from the others is the most compelling. I don't think the context was ever really considered originally, but having it consistent is quite useful at least.

@dougwilson dougwilson added this to the 4.18 milestone Mar 26, 2022
@dougwilson dougwilson mentioned this pull request Mar 26, 2022
20 tasks
@dougwilson dougwilson changed the base branch from master to 4.18 March 26, 2022 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants