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

fix: handle observable() w/o getOwner #1115

Merged
merged 1 commit into from
Jul 14, 2022
Merged

fix: handle observable() w/o getOwner #1115

merged 1 commit into from
Jul 14, 2022

Conversation

jorroll
Copy link
Contributor

@jorroll jorroll commented Jul 13, 2022

Updates the observable() function to support usage outside the Solidjs context. Also updates ObservableObserver to make the "next" property in an observer object optional to align with RXJS's Subscribable interface (which is what RXJS's from() expects). At the moment, from(observable(mySignal)) results in a type error (assuming from() is imported from RXJS).

Summary

Fixes #1110

I'll note that the "Before submitting a pull request" instructions include "Format your code with prettier" yet the Solidjs repo doesn't include a formatting script nor is prettier listed as a devdependency? I attempted to run yarn prettier but since prettier wasn't available I gave up.

How did you test this change?

I reimplemented the observable() function in my React app using this solution.

@coveralls
Copy link

coveralls commented Jul 13, 2022

Pull Request Test Coverage Report for Build 2668977635

  • 10 of 12 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 88.876%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/solid/src/reactive/observable.ts 10 12 83.33%
Totals Coverage Status
Change from base Build 2664305051: -0.01%
Covered Lines: 1257
Relevant Lines: 1341

💛 - Coveralls

Updates the observable() function to support usage outside a Solidjs context. Also updates ObservableObserver to make the "next" property in an observer object optional to align with RXJS.
fixes solidjs#1110
// properly typed for 3rd party library's like RXJS.
declare global {
interface SymbolConstructor {
readonly observable: symbol;
Copy link
Contributor Author

@jorroll jorroll Jul 14, 2022

Choose a reason for hiding this comment

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

As the comment above this interface suggests, ensuring that a type for Symbol.observable exists is necessary for Solid's observable() function to be properly typed for RXJS (and I imagine other observable library's as well). This is because RXJS's from() function expects an object with { [Symbol.observable](): Subscribabble<T> }. Previously, getSymbol() didn't return Symbol.observable but instead returned any which breaks type interoperability.

@ryansolid
Copy link
Member

Looks good. Thank you.

@ryansolid ryansolid merged commit 03444b0 into solidjs:main Jul 14, 2022
@ryansolid
Copy link
Member

I do have one more thought here. Disposal is tied to the context where the subscription happens being cleaned up, but I'm sort of wondering if the disposal should tied to the observable wrapper itself. Very much like we were talking about it maybe should be a single computed. It really could go either way because it's the hot versus cold thing. But Solid is Hot, and perhaps we should treat our Observables here as hot too, basically they are Subjects rather than Observables.

@jorroll do you have an opinion.

@jorroll
Copy link
Contributor Author

jorroll commented Jul 15, 2022

Disposal is tied to the context where the subscription happens being cleaned up, but I'm sort of wondering if the disposal should tied to the observable wrapper itself.

Ya, I thought about that in the context of creating an observable that shares a single computed. My example SolidObservable potentially changed the Solidjs Owner associated with each subscription (vs the current implementation).

It really could go either way because it's the hot versus cold thing.

Correct me if I'm wrong, I was thinking that a "hot" observable is one that has a side-effect/does some work when the observable is created whereas a "cold" observable just has a side effect/does some work when it is subscribed to. With this definition, hot vs cold doesn't necessarily enter this discussion.

For example the SolidObservable implementation I wrote for "an observable which shares a single computed" doesn't create that computed when the observable is created. Instead, that SolidObservable captures the current owner context when the observable is created but delays creating the computed until someone subscribes for the first time. It then tracks subscribers and disposes of the computed if everyone unsubscribes (only to create a new computed if someone subscribes again). So it's sharing a single computed but it's still a cold observable. Obviously the implementation could be "hot" though (and would be a little simpler to implement that way). It's not obvious to me that theres any need for this observable to be "hot" though?

@jorroll do you have an opinion.

I'm interpreting this question as "should the observable capture the owner when it's created or should it capture the owner at subscription time" and honestly I'm not really sure. I can see it going either way. But if I had to pick one, capturing the owner at observable creation time seems like it will probably be the least surprising to the most people.

@ryansolid
Copy link
Member

I agree with pretty much everything you've said here. Yeah doing creation lazily is better in terms of creation. I use the hot/cold pretty loosely I suppose, similar to Andre Staltz' justification for creating XStream. In any case I think that approach is probably the correct approach. And the direction this should go ultimately. Thank you for confirming this.

@jorroll
Copy link
Contributor Author

jorroll commented Jul 15, 2022

Ah, from the article:

RxJS Observables are generally cold. This means that two different calls to subscribe() will generate two separate side effect free executions of the Observable. Hot, on the other hand, is when different calls to subscribe() may share the same execution of the Observable.

In this case, and with deciding to capture the Owner at observable creation time rather than subscription time, moving towards hot observables sounds like the right idea.

In any case I think that approach is probably the correct approach. And the direction this should go ultimately. Thank you for confirming this.

👍

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.

Using observable() outside of a createRoot() or render() call doesn't behave as expected
3 participants