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

Adds support for replacer in json/jsonp calls #2098

Closed
wants to merge 4 commits into from
Closed

Adds support for replacer in json/jsonp calls #2098

wants to merge 4 commits into from

Conversation

gvaish
Copy link

@gvaish gvaish commented May 4, 2014

See #2097

Gaurav Vaish added 2 commits May 4, 2014 10:10
* `lib/response.js`: Updated `res.json` and `res.jsonp` to accept optional replaced, updated doc
* `test/res.json.js`: Added test cases for (object, replacer), (status, object, replacer) and (object, status, replacer)
* `test/res.jsonp.js`: Added test cases for (object, replacer), (status, object, replacer) and (object, status, replacer)
* `test/res.json.js`: Call `json` and not `jsonp`. Copy-paste side effects
@@ -161,28 +161,40 @@ res.send = function(body){
* res.json({ user: 'tj' });
* res.json(500, 'oh noes!');
* res.json(404, 'I dont have that');
* res.json({ user: {id: 'id01', pwd: 'pass'}, id: 'id01', name: 'gvaish' }, ['id', 'name']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not add your GitHub username to our examples, please.

@gvaish
Copy link
Author

gvaish commented May 4, 2014

Updated the example to remove my username to something generic.

@dougwilson
Copy link
Contributor

@gvaish thanks, but your username is still there in the second example.

@gvaish
Copy link
Author

gvaish commented May 4, 2014

Grrr... my bad. Fixed that too. :-)

if ('number' == typeof replacer) {
this.statusCode = replacer;
replacer = arguments[2];
} else if('number' == typeof obj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ( everywhere plz

@jonathanong
Copy link
Member

i just don't like how complex it makes the function due to all the type checking. very little people use this as well. why not just stringify it yourself?

@gvaish
Copy link
Author

gvaish commented May 5, 2014

Agree to the "type checking" point. But that's primarily because of supporting status, obj and obj and obj, status which was already in there.

Point here is - in general, the core APIs surface content (model) while the end-points (controller) filter out the content.

@gvaish
Copy link
Author

gvaish commented May 6, 2014

@jonathanong @dougwilson - Any luck (for this PR to make it through)?

@jonathanong
Copy link
Member

Up to the others if they want to merge it. I'm -1 because it's complex and wouldn't be used very often by most people

@nook-scheel
Copy link

+1

@defunctzombie
Copy link
Contributor

What is the point of this?
On May 4, 2014 10:28 AM, "Gaurav Vaish" notifications@github.com wrote:


You can merge this Pull Request by running

git pull https://github.com/gvaish/express master

Or view, comment on, or merge it at:

#2098
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com//pull/2098
.

@dougwilson
Copy link
Contributor

@gvaish wants to use the replacer function to sort the keys in the resulting JSON, but doesn't want to do it for all requests for some reason.

This doesn't particularly sound like a valid use for the replacer function, anyway, because you cannot sort the top-level object keys with it, let alone guarantee any key ordering for sub-objects since the ECMAscript spec does not guarantee key order (but you can make it look like it if you return an object where the keys where inserted alphabetically since currently v8 will return keys in insertion order).

The replacer function is only to return altered values or to omit values, it cannot technically be used to sort property keys in objects.

@dougwilson dougwilson closed this May 11, 2014
@expressjs expressjs locked and limited conversation to collaborators Oct 29, 2014
@dougwilson dougwilson added the pr label Oct 29, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants