Skip to content

Conversation

@OttoHatt
Copy link
Contributor

@OttoHatt OttoHatt commented Jul 2, 2023

  • Allow returning nil from the observable. We know that it will never evaluate to anything interesting, so there's no point creating a brio (which are not free). (Should also probably skip creating a brio if the passed observable is complete and evaluates to nil, i.e. == Rx.EMPTY?).
  • Propagate state of passed observable. This has just become a slightly more efficient version of a switchMap + toBrio.
  • Returning from a subscription does nothing.

sub:Fire(current)

return current
end, function(...)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can pass sub:GetFailComplete() here. Note I think it's fine to complete this here, but a failure could cause other observables in the blend tree to also fail. Most of Nevermore doesn't really leverage the failure state that much.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think it's safe to merge this, I'm down for it.

Copy link
Contributor Author

@OttoHatt OttoHatt Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted the completion, I didn't realise this would could pass failures. I was only vaguely aware subscriptions had this - I know for certain I'm not handling it!

Though it's a breaking change, I would want to propagate this. I think hiding errors is bad. But silently failing is also bad, because I'm not handling this case and wouldn't know why my subscription stopped. I think subscriptions should print a warning (similar to promises) for an unhandled failure state (i.e. no failCallback), and then this should be merged?

@Quenty
Copy link
Owner

Quenty commented Jul 10, 2023

Side note: Rx.EMPTY never completes I believe.

@OttoHatt
Copy link
Contributor Author

If propagating failure is contentious, is it possible to merge just the first of these commits for now?

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