[v3] don't wrap events in slice #4181
Conversation
WalkthroughThe pull request updates the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
|
Hi! I included a similar change in my PR #4161 but an ensuing discussion on discord left me wondering whether it would make for poor DX. The unexpected slice is definitely bad though, plus it doesn't match the behaviour of the JS runtime, for added confusion. I was thinking of pushing an update to my PR proposing to emit multiple events as you too suggest, but that would break people's existing code silently as opposed to compile time errors.. not an easy tradeoff. |
|
Now that I think of it, emitting multiple events will still cause runtime errors when the data field is asserted to a slice type, so either approach should be safe. |
|
I definitely do not like passing a structure and getting a slice of object out on the JS side. |
|
I think I found a better solution than both ones discussed above:
This approach fixes the counterintuitive behaviour in the most common case (one argument) without breaking things for people who might be passing in multiple arguments (I expect there to be a smallish but non-zero number of them). At the same time, it avoids the potentially confusing behaviour of emitting multiple events (one per argument) from a method called I included the change in #4161 but I do not consider that PR ready for merging. Feel free to copy the implementation from there if you like the approach. |
|
I would argue the most intuitive thing to do with this code is to send exactly what the caller provided. IE if they provided a nil send the nil. This is the principle of least surprise, the same object in the same object out to the best of our ability. |
|
I agree with you in principle. There were complaints on Discord about not being able to omit a
My proposal was meant to address those complaints. I do believe that app.EmitEvent("myevent", nil)is a perfectly acceptable level of boilerplate, the simplest option and therefore The Go Way™. |
|
Yes that is what I was meaning. Send whatever argument given. If they want a no argument event we could have one more method for that. EmitEmptyEvent to make it obvious |
|
Wait is that the point of the variadic? To allow it to not be specified at all? Guess that makes it hard to check for not provided versus actually sending nil. |
|
I just want it to not become a slice inadvertently. |
Just syntactic sugar for passing
Given that the |
This single commit removes the
variadicoperator from the input toEmitEvent.Using a variadic operator for 'data' causes the
eventobject to be wrapped in a single element slice which forces the consumer of the event to operate on a slice of messages even if only a single non-slice object was submitted to the function.If the
variadic operatoris intended it would seem that an iteration over the elements might be required and to send multiple events in that case.Looking for clarification on desired functionality here if this is the wrong solution.
Summary by CodeRabbit