search

expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.65k stars 16.26k forks source link

Deprecate res.send(obj) json overload #2249

Open dougwilson opened 10 years ago

dougwilson commented 10 years ago

This is an idea for removing the res.send(obj) signature and it's other similar signatures that will JSON.stringify the value and send it as JSON. In my opinion there is no reason you cannot just type res.json instead. Deprecating and removing this overload will mean people do not have to make sure their variable to res.send is a string to get HTML output.

Fishrock123 commented 10 years ago

Sounds reasonable.

jonathanong commented 10 years ago

meh i would just leave it. makes it easier for newer people.

make sure their variable to res.send is a string to get HTML output.

what does this mean? if anything, we could deprecate res.send(Buffer) since i don't see the point of doing that. supporting only strings or JSON objects doesn't sound unreasonable.

dougwilson commented 10 years ago

what does this mean?

It means

renderPage(function(err, page){
  if (err) return next(err)
  res.send(page)
})

will suddenly send JSON if that function returned something weird by mistake (like a null). In res.send, null and undefined actually have two different behaviors because of the JSON stuff, which res.end does not suffer from (but res.end does not give automatic conditional responses and ETags).

we could deprecate res.send(Buffer) since i don't see the point of doing that

No, I disagree. Using a Buffer is necessary if you are going to send your HTML in a different charset than UTF-8 (using something like iconv to encode). I do think that Buffer values should probably set the content-type to text/html, though. If res.send did not accept Buffer, then we'd need to add another method that did, because it's the way you get ETag and conditional responses for free.

TL;DR I think res.send should just send text/html out, just like res.json only does application/json, instead of mixing a bunch of things together.

jonathanong commented 10 years ago

I just think it's unnecessary API breakage. Other than that, it doesn't really matter to me

dougwilson commented 10 years ago

Right, it's just a thought right now. If we did make this change, though, you could do neat stuff like this:

res.get('/', function (req, res) {
  var page = new Page()
  res.send(page)
})

function Page() {
}
Page.prototype.toString = function toString() {
  // return the constructed page contents
  return '<html>' + this.data + '</html>'
}
defunctzombie commented 10 years ago

+1 for not doing magic in .send for objects. Calling toString seems sensible over automagically returning JSON.

dougwilson commented 10 years ago

Overall, I think forcing the arg to a string and sending only HTML is a good answer (especially since we have res.json already for a long time), but I was thinking on @jonathanong 's point and thinking maybe to not deprecate res.send(obj) until after 5.0 is released so it would be scheduled for removal in a 6.0, idk yet.

jonathanong commented 10 years ago

i like the Page() idea. we could do some crazy stuff and check if (typeof obj === 'object' && obj.constructor === Object) and only JSON.stringify() if that's the case. hahaha

dougwilson commented 10 years ago

Though people may be sending in objects right now that instead have a toJSON on their prototype, since it's getting passed to JSON.stringify right now.

hacksparrow commented 10 years ago

+1 for the sake of consistency. If you want to send json, you have res.json(). Best to have one interface for one thing.

jarradseers commented 10 years ago

This is an interesting topic. It's nice to be able to just res.send whatever I like and it just gets handled but at the same time res.json is more accurate and simplifies the res.send if object handling were removed.

One use case I have for keeping it in res.send is if I use a bunch of middleware that will append / overwrite an attribute res.output to either be:

I don't have to check the type in the final middleware when I do res.send(res.output). At the same time, it's not hard to do.

So +1 for removing object detection in res.send.

Kind regards, Jarrad Seers 021 809 770

On 21/07/2014, at 8:40 am, Jonathan Ong notifications@github.com wrote:

I just think it's unnecessary API breakage. Other than that, it doesn't really matter to me

— Reply to this email directly or view it on GitHub.

rlidwka commented 10 years ago

Maybe allow user to register some kind of a callback to format res.send(object) result? So if he wants magic to happen, he could register a function that serializes objects to json.

I know it could be done with monkey-patching already, but it would be great to have a better way.

dougwilson commented 10 years ago

Maybe allow user to register some kind of a callback to format res.send(object) result? So if he wants magic to happen, he could register a function that serializes objects to json.

What would be the advantage over calling res.json here?

rlidwka commented 10 years ago
  1. one interface for returning anything. Instead of remembering res.json, res.jsonp, res.whatever a developer would have to just use res.send() everywhere
  2. it is more easily configurable, if I want to tweak JSON.stringify() arguments (space or replacer), it's easier to register a callback than to read documentation trying to find how to set up each of those arguments in settings
rlidwka commented 10 years ago

In fact, I even thought about second middleware stack that's executed by res.send() where you can define functions to manipulate result of res.send() (for example, forming JSONP output or doing JSON.stringify()).

dougwilson commented 10 years ago

In fact, I even thought about second middleware stack that's executed by res.send() where you can define functions to manipulate result of res.send() (for example, forming JSONP output or doing JSON.stringify()).

Hmm. Perhaps open a new issue for this and I can label it as "Idea" for discussion from everyone :) Ideally this pipeline would not be tied to res.send, but include res.end and all other ways to write out data (seems like it would be a good use-case for people wanting to do HTML-injection, URL-rewriting, etc.).

I don't think that would particularly overrule this idea, since I think you idea, if implemented, should be more generalized than being specific to res.send. A lot of middleware, for example, do not use res.send since it is an express thing and they want to work with connect and vanilla Node.js servers.

jimmywarting commented 10 years ago

Developer should stop using JSONP and go for CORS instead

JSONP is unsafe and overly complicated to maintain on both backend and frontend. now days its very easy cuz we have this awesome framework that dose it for us :(

If we should have any shortcut to sending json cross domain it would be something with res.cors().json(obj) or with the help of middleware i would not encourage new user to use res.jsonp(). If they were using it then I would show a warning message to tell them a reason why, how to solve, and why its being deprecated.

dougwilson commented 10 years ago

Not sure how the comment relates to the original post (which does not mention JSONP at all). Let's stay on topic, please :)

jimmywarting commented 10 years ago

Sorry, went to far. properly wouldn't have mention it if it weren't for @rlidwka who brought up jsonp in one of his message. no offence