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

Please support events without event names #14

Closed
joshtriplett opened this issue Dec 5, 2020 · 6 comments
Closed

Please support events without event names #14

joshtriplett opened this issue Dec 5, 2020 · 6 comments
Assignees

Comments

@joshtriplett
Copy link
Member

The server-sent events standard does not require the event field. I'd like to be able to send events with no event field.

I can think of several possible ways to do this:

  • Modify the send method to take an Option<&str>. This would be the simplest to implement, but would necessitate a major version bump, and would require passing Some("name") when providing an event name, which may be inconvenient for the common case.
  • Add a new method that omits the argument for the event name. This would be backwards-compatible, but would require a more cumbersome method name when sending data-only events.
  • Add a new method taking an Option<&str>, and keep the existing method as a convenience wrapper.

I'd be happy to provide the implementation. Which approach would you prefer?

@jbr
Copy link
Member

jbr commented Dec 5, 2020

@yoshuawuyts ☝️

@yoshuawuyts
Copy link
Member

Hmm, I didn't expect people to wanr to send messages without an event name, but I guess that's proven to be overly limiting.

I think the first option would be the most straightforward; issuing a semver major seems fine for this as well.

Though I wonder: if we make the argument impl Into<Option<&str>>, that should work with both string literals and None right? If that works I don't believe that would be a breaking change since all previous inputs are accepted, but it's possible to pass None to omit the name. (Not near a computer right now so can't confirm if this works)

@joshtriplett
Copy link
Member Author

Hmm, I didn't expect people to wanr to send messages without an event name, but I guess that's proven to be overly limiting.

For short messages, event: xyz adds substantial overhead.

Though I wonder: if we make the argument impl Into<Option<&str>>, that should work with both string literals and None right? If that works I don't believe that would be a breaking change since all previous inputs are accepted, but it's possible to pass None to omit the name. (Not near a computer right now so can't confirm if this works)

That's a good idea, and it'd be much easier for people to use. It would not technically be a breaking change by the normal compatibility rules, but it could potentially break code that was relying on inference. I think it'd still be a good idea to do a major version bump for it, but most people should just be able to recompile their code. I'll implement that.

@joshtriplett
Copy link
Member Author

Fixed in #15

@jbr
Copy link
Member

jbr commented Dec 5, 2020

released in v5.0.0 — I think it should be possible to use a patch.crates-io to use this with tide 0.15, but I haven't confirmed that. If not, tide bump will probably wait for next tide minor

@jbr jbr closed this as completed Dec 5, 2020
@jbr
Copy link
Member

jbr commented Dec 5, 2020

ref: http-rs/tide#760

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

No branches or pull requests

3 participants