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

Update Jetty9 server adapter to ring-jetty9-adapter's new websocket fix #408

Closed
wants to merge 11 commits into from

Conversation

surferHalo
Copy link
Contributor

Currently, ring-Jetty9-adapter does not regard websocket as a separate ring handler.
sunng87/ring-jetty9-adapter#59

ptaoussanis and others added 3 commits July 25, 2022 17:50
Currently, I can't find a way to response a {:body sch} inside ring-req->server-ch-resp.
@ptaoussanis ptaoussanis self-assigned this Jul 29, 2022
@ptaoussanis ptaoussanis added this to the v1.18 milestone Sep 20, 2022
@ptaoussanis
Copy link
Member

ptaoussanis commented Feb 23, 2023

@surferHalo Hi Jian, thank you for your PR - and sorry for the long delay replying on this!

In your PR, you have the following code:

(defn- server-ch-resp
  [websocket? {:keys [on-open on-close on-msg on-error]}]
  (jetty/ws-upgrade-response
   {:on-connect (fn [sch]          (on-open  sch websocket?))
    :on-text    (fn [sch msg]      (on-msg   sch websocket? msg))
    :on-error   (fn [sch error]    (on-error sch websocket? error))
    :on-close   (fn [sch status _] (on-close sch websocket? status))}))

I'm not familiar with this Jetty9 adapter, but it looks strange to me that the server-ch-resp return value will use jetty/ws-upgrade-response even if the websocket? argument is false.

Is this really correct? Ajax will work correctly even when websocket? is false?

Also, you mention in your commit message:

Currently, I can't find a way to response a {:body sch} inside ring-req->server-ch-resp.

I don't understand. Can you explain a bit more why you wrote this?

Thanks!

@ptaoussanis
Copy link
Member

Closing to consolidate efforts at #424

@ptaoussanis ptaoussanis closed this Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants