salsita / spicy-hooks

7 stars 0 forks source link

Rethink `observables/useSyncObservable` #13

Open goce-cz opened 3 years ago

goce-cz commented 3 years ago

I think we need to reconsider the name and general usefulness of this hook.

The beauty of BehaviorSubject lays in its ability to provide the latest emitted value through getValue() method. This gives us possibility to initialize React state directly with a value rather than waiting for the subject to emit. Doing useSnapshot() can therefore provide an immediate value directly during the first render, when you pass a BehaviorSubject to it.

Unfortunately, pipeing the subject doesn't pipe the getValue() function, therefore even if the pipeline itself is synchronous we lose the possibility to grab the latest (and transformed) value.

useSyncObservable solves this by subscribing to an observable in a synchronous fashion. If the observable itself is synchronous it will emit the value immediately, thus we can grab it and pass it through a newly created BehaviorSubject. The calling code can then reach it through the standard getValue().

I currently see following issues:

  1. the name is totally confusing
  2. the TSDoc as well
  3. it can trigger "side-effects" during a render
    • first call to the hook will immediately subscribe to the observable
    • if the observable is cold, this will trigger the internal logic and start emitting
    • it is generally considered to be a bad practice to initiate side-effect during the render phase
      • however the concept of resources and suspense is IMO encouraging this as the first call to resource.read() (or similar) will always trigger the side-effect
      • so I'm not sure how much of a big deal this actually is 🤷
  4. it is used only by useSnapshot
    • I'm not sure how useful this hook actually is outside of the scope of useSnapshot
    • maybe we could merge these (or better make this one private)
    • a completely different way would be to remove the sage of useSyncObservable from useSnapshot and make it optional - i.e. let the user do useSnapshot(useSyncObservable(source)) manually, but I don't know if I like this

Any opinion more than welcomed...

Bartunek commented 3 years ago

I guess we can make this private part of useSnapshot's implementation and only reconsider exposing it if such a need arises in the future.