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

Already on GitHub? Sign in to your account

JSONP #1

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

sunng87 commented Apr 7, 2012

Added jsonp support.

Owner

weavejester commented Apr 7, 2012

Rather than trying to put two different sets of functionality in the same function, I think it would make more sense to have two functions. Maybe a json-callback function.

Also, could you make the commit message a little more descriptive?

sunng87 commented Apr 7, 2012

Personally, I think it's convenience to combine them into a single function to support both json and jsonp API. For example:

(defn my-json-api [req]
  (let [calback (-> req :params :callback)
        data ...]
    (json-response data :callback callback)))

This could enable support for jsonp seamlessly.

Owner

weavejester commented Apr 7, 2012

It is more convenient for certain tasks, but not simpler. I prefer simplicity over convenience where possible.

qerub commented Jun 12, 2012

I'd very much like to see JSONP support in this library. Any chance you could reach a consensus on how to implement this? Would you accept a commit that added a jsonp-response function?

qerub commented Jun 12, 2012

A better idea: Let's implement JSONP support with middleware.

sunng87 commented Jun 12, 2012

I think in most cases, we would like to support JSON and JSONP at the same time. So I think it makes sense to put "callback" as an option.

And there is one more thing to care about: In JSON rpc application, we can use HTTP status code to indicate the message type. But in JSONP, we have to make sure the status is always 200. Otherwise, the function would never be executed.

qerub commented Jun 12, 2012

OK, here's some real code: 332f3ff. This middleware makes it trivial to support JSONP. Just use it and your JSON responses can automatically be transformed to JSONP.

Owner

weavejester commented Jun 12, 2012

GitHub isn't allowing me to view this issue when I'm logged in, so I can't comment on this beyond sending emails until they fix it, which kinda sucks.

However, be aware that in idiomatic Clojure we tend to prefer simplicity over convenience. Two separate functions should not be merged into one if we can help it, so we should have with a jsonp-response or json-callback function.

It's also not the case that if we want JSON we automatically want JSONP.

qerub commented Jun 13, 2012

It's also not the case that if we want JSON we automatically want JSONP.

The middleware can be applied to only the handlers that should have
JSONP support.

Owner

weavejester commented Jun 13, 2012

I can access the issue again :)

The middleware can be applied to only the handlers that should have
JSONP support.

Sorry, I was responding to sunng87, but I didn't make that clear. Using middleware to wrap a JSON response is an interesting idea, but I wonder if it's too specific. Let me think about this.

qerub commented Jun 27, 2012

What are your current thoughts on adding wrap-json-with-padding to ring-json-response? Should I perhaps distribute it through a new project?

Owner

weavejester commented Jun 27, 2012

Sorry, I've been a little busy lately to give this the attention it deserves.

I think my inclination is that it should maybe be another project. JSONP seems like something on top of JSON support, perhaps. However, I'm not entirely certain of my position; I'm a little on the fence here.

qerub commented Jun 28, 2012

No problem.

All right; I'll package the JSONP middleware as its own project […when I get less busy ;)]. If you ever decide you'd like the functionality in ring-json-response, we can merge.

qerub commented Jul 7, 2012

I decided to not create yet another project for this simple function, but I factored it out and published it here: https://gist.github.com/3068262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment