feat(rxjs): Convert atomWithObservable to Observable spec #593
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/EVJw56QvTA2vdhcesnZKRicXHbhY |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 938c147:
|
| import { atom } from 'jotai' | ||
| import type { Atom, WritableAtom, Getter } from 'jotai' | ||
|
|
||
| const observableSymbol: typeof Symbol.observable = | ||
| typeof Symbol === 'function' |
There was a problem hiding this comment.
Symbols are used everywhere, so we wouldn't need to support non-Symbol env specific here.
| import { atom } from 'jotai' | ||
| import type { Atom, WritableAtom, Getter } from 'jotai' | ||
|
|
||
| const observableSymbol: typeof Symbol.observable = | ||
| typeof Symbol === 'function' | ||
| ? Symbol.observable || ((Symbol as any).observable = Symbol('observable')) |
There was a problem hiding this comment.
I guess it'd be better to use Symbol.for() for the fallback? Or, is this the de-facto for the fallback?
There was a problem hiding this comment.
After looking at the code, it's basically used only in L46-L47.
So, this could just be:
const observableSymbol = Symbol.observable || undefined(meaning we don't need this)
Or, is there any specific reason we want to assign Symbol.observable with a unique symbol?
There was a problem hiding this comment.
I think it's an attempt to polyfill the Symbol.observable: https://github.com/benlesh/symbol-observable/blob/master/es/ponyfill.js
I'm not sure if it's the correct implementation.
There was a problem hiding this comment.
Thanks. In such a case, const observableSymbol = Symbol.observable should be fine for us, and if really required, people should polyfill (not ponyfill) the symbol in their app code.
| subscription = observable.subscribe((data) => { | ||
| dataListener(data) | ||
| if (subscription && !setResult) { | ||
| subscription.unsubscribe() | ||
| subscription = null | ||
| } | ||
| }, errorListener) |
There was a problem hiding this comment.
Does this work like first()? We would need to unsubscribe on error too.
| if (!resolve) { | ||
| subscription.unsubscribe() | ||
| subscription = null | ||
| } |
There was a problem hiding this comment.
the reason this would be required is subscription callback is invoked in sync, but then it's already unsubscribed.
hm, what's the intention behind this?
I like the idea very much! If this works,
Sounds good. I assume you are expert on this topic. |
|
Haha, I wouldn't call myself an expert, but I believe this would work with most Observable livraries and implementations. I'd still say though that there may be some that will cause edge cases, but that remains to be seen. |
Yeah, probably. That said, if you are fine, I'd like to merge this and continue some tweaks on my end. |
|
Or, I can work on this branch, if you prefer. |
|
@dai-shi either works for me ✌️ I think I'll have to test some trampoline scheduled stuff to make sure |
This is a draft / WIP.
Ref: #234
This converts
jotai/rxjsover to a genericatomWithObservablethat potentially supports allObservablelibraries that adhere to the Observable proposal.To replace
firstthe initial subscription unsubscribes at the earliest possible time, which takes synchronous (non-deferred) results into account. TheresultAtom.onMountfunction will also now reuse the subscription if possible and otherwise start a new subscription, to match the previous implementation closely in its double-subscription behaviour.