{"action":"created","issue":{"url":"https://api.github.com/repos/tipsy/javalin/issues/273","repository_url":"https://api.github.com/repos/tipsy/javalin","labels_url":"https://api.github.com/repos/tipsy/javalin/issues/273/labels{/name}","comments_url":"https://api.github.com/repos/tipsy/javalin/issues/273/comments","events_url":"https://api.github.com/repos/tipsy/javalin/issues/273/events","html_url":"https://github.com/tipsy/javalin/pull/273","id":334954044,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk2Nzk1MzEx","number":273,"title":"Add Server Sent Event Feature","user":{"login":"7agustibm","id":8149332,"node_id":"MDQ6VXNlcjgxNDkzMzI=","avatar_url":"https://avatars0.githubusercontent.com/u/8149332?v=4","gravatar_id":"","url":"https://api.github.com/users/7agustibm","html_url":"https://github.com/7agustibm","followers_url":"https://api.github.com/users/7agustibm/followers","following_url":"https://api.github.com/users/7agustibm/following{/other_user}","gists_url":"https://api.github.com/users/7agustibm/gists{/gist_id}","starred_url":"https://api.github.com/users/7agustibm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/7agustibm/subscriptions","organizations_url":"https://api.github.com/users/7agustibm/orgs","repos_url":"https://api.github.com/users/7agustibm/repos","events_url":"https://api.github.com/users/7agustibm/events{/privacy}","received_events_url":"https://api.github.com/users/7agustibm/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":12,"created_at":"2018-06-22T17:09:52Z","updated_at":"2018-12-27T10:52:17Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/tipsy/javalin/pulls/273","html_url":"https://github.com/tipsy/javalin/pull/273","diff_url":"https://github.com/tipsy/javalin/pull/273.diff","patch_url":"https://github.com/tipsy/javalin/pull/273.patch"},"body":"Create the new function in Context class for send event from server to front. Reference: #91 "},"comment":{"url":"https://api.github.com/repos/tipsy/javalin/issues/comments/450128331","html_url":"https://github.com/tipsy/javalin/pull/273#issuecomment-450128331","issue_url":"https://api.github.com/repos/tipsy/javalin/issues/273","id":450128331,"node_id":"MDEyOklzc3VlQ29tbWVudDQ1MDEyODMzMQ==","user":{"login":"tipsy","id":1521451,"node_id":"MDQ6VXNlcjE1MjE0NTE=","avatar_url":"https://avatars3.githubusercontent.com/u/1521451?v=4","gravatar_id":"","url":"https://api.github.com/users/tipsy","html_url":"https://github.com/tipsy","followers_url":"https://api.github.com/users/tipsy/followers","following_url":"https://api.github.com/users/tipsy/following{/other_user}","gists_url":"https://api.github.com/users/tipsy/gists{/gist_id}","starred_url":"https://api.github.com/users/tipsy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tipsy/subscriptions","organizations_url":"https://api.github.com/users/tipsy/orgs","repos_url":"https://api.github.com/users/tipsy/repos","events_url":"https://api.github.com/users/tipsy/events{/privacy}","received_events_url":"https://api.github.com/users/tipsy/received_events","type":"User","site_admin":false},"created_at":"2018-12-27T10:52:17Z","updated_at":"2018-12-27T10:52:17Z","author_association":"OWNER","body":"Good work @7agustibm, sorry this has taken so long to review. A few notes:\r\n\r\n# API\r\n\r\n## Current in PR\r\n\r\n```kotlin\r\napp.sse(\"/sse\") { sse ->\r\n    sse.onOpen { eventSource ->\r\n        eventSource.sendEvent(\"connect\", \"Connected!\")\r\n        eventSources.add(eventSource)\r\n    }\r\n    sse.onClose { eventSource -> eventSources.remove(eventSource) }\r\n}\r\n```\r\n\r\n## My suggestion\r\nSince we use `onConnect` and `Session` with WebSocket, I'm thinking we should use that here too:\r\n\r\n```\r\napp.sse(\"/sse\") { sse ->\r\n    sse.onConnect { session ->\r\n        session.sendEvent(\"connect\", \"Connected!\")\r\n        eventSourceSessions.add(session)\r\n    }\r\n    sse.onClose { session -> eventSourceSessions.remove(session) }\r\n}\r\n```\r\n\r\nThere is something strange with the methods available to `sse` and `eventSource`. They're both the same interface, so you can do: \r\n\r\n```kotlin\r\nsse.onOpen { eventSource ->\r\n    eventSource.sendEvent(\"connect\", \"Connected!\") // send to connecting client?\r\n    eventSource.onOpen {} // what does this do?\r\n}\r\nsse.sendEvent(\"connect\", \"Connected!\") // send to who?\r\n```\r\n\r\n`Sse` and `EventSource` have to be two difference interfaces with only the appropriate methods.\r\n\r\n# Implementation\r\n\r\n* There is a `pathParams()` method. I would remove this and give access to the `Context`. The handler is implemented on a standard `app.get` endpoint, so `Context` could easily be available.\r\n* I would rename `EmitterImpl` and `EventSourceImpl` to `Emitter` and `EventSource`, and get rid of the interfaces. Javalin has no other interace/impl constructs. This would also mean getting rid of the mock-tests, and rely fully on integration tests.\r\n* Check if it's possible to use https://github.com/jetty-project/jetty-eventsource-servlet/wiki instead of writing all the custom code. The dependency can be found here: https://mvnrepository.com/artifact/org.eclipse.jetty/jetty-servlets\r\n* There are several things named `SSEThing`. Javalin follow strict camelcasing, so it should be `SseThing`\r\n"}}
{"actor":{"display_login":"tipsy"}}