firebase
[Go] Refactored `Stream()` API to be more ergonomic.
The previous API is not ideal because you have to pass in a function into an iterator-like interface that then chooses between stream value, final value, or error. This change resembles a classic iterator.
Checklist (if applicable):
- [x] Tested (manually, unit tested, etc.)
- [ ] Changelog updated
- [x] Docs updated
The general feeling is that designs that only report the error at the end are error-prone (no pun intended).
The classic example is https://pkg.go.dev/bufio#Scanner, where to make the iteration convenient, you have to call a function at the end to get the error. We know empirically, from looking at a lot of code, that people often forget to make that call to Err.
You are in a better place here, because at least FinalOutput is returning something useful as well as the error. If you think everyone will want that final value, then this is fine. If you think some people might not need it, you may have a problem. If you do go down this road (I'm not going to try and stop you beyond this comment), make sure docs and examples emphasize the need to check that error even if the final output isn't wanted.
One other thing to think about: Go 1.23 is about to drop, and with it support for iterators, or as we call it, range-over-func. See https://eli.thegreenplace.net/2023/preview-ranging-over-functions-in-go and https://tip.golang.org/doc/go1.23. Now, the Go team promises support for the last two versions of Go, so I recommend you do too. That means you shouldn't use this feature until Go 1.24 comes out in about six months. But it's worth thinking about how you'd design your iterators with range-over-func.
The general feeling is that designs that only report the error at the end are error-prone (no pun intended). The classic example is https://pkg.go.dev/bufio#Scanner, where to make the iteration convenient, you have to call a function at the end to get the error. We know empirically, from looking at a lot of code, that people often forget to make that call to
Err.You are in a better place here, because at least FinalOutput is returning something useful as well as the error. If you think everyone will want that final value, then this is fine. If you think some people might not need it, you may have a problem. If you do go down this road (I'm not going to try and stop you beyond this comment), make sure docs and examples emphasize the need to check that error even if the final output isn't wanted.
My first stab at this (https://github.com/firebase/genkit/pull/766/commits/f62be7a31a40711f50cc289f256531b3d711533a) included returning an error from Next() so that the caller can be notified if it fails mid-stream. In many cases, users will want the final output so I don't find it that risky or strange to have to call it to get the final result, whether it's good output or an error. I agree with you it should be well documented that it needs to be checked.
One other thing to think about: Go 1.23 is about to drop, and with it support for iterators, or as we call it, range-over-func. See https://eli.thegreenplace.net/2023/preview-ranging-over-functions-in-go and https://tip.golang.org/doc/go1.23. Now, the Go team promises support for the last two versions of Go, so I recommend you do too. That means you shouldn't use this feature until Go 1.24 comes out in about six months. But it's worth thinking about how you'd design your iterators with range-over-func.
@pavelgj
For us it would look like this, which is pretty clean:
iter := DefineStreamingFlow(...)
for s, err := range iter {
if err != nil {
// handle err
}
if s.Done {
output := s.Output
} else {
chunk := s.Stream
}
}
