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

Rethink observables/useSyncObservable #13

Open
goce-cz opened this issue Oct 8, 2020 · 1 comment
Open

Rethink observables/useSyncObservable #13

goce-cz opened this issue Oct 8, 2020 · 1 comment
Assignees
Labels
help wanted Extra attention is needed weirdness Something is Confusing or unclear
Milestone

Comments

@goce-cz
Copy link
Collaborator

goce-cz commented Oct 8, 2020

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...

@goce-cz goce-cz added weirdness Something is Confusing or unclear help wanted Extra attention is needed labels Oct 8, 2020
@goce-cz goce-cz added this to the v2.0.0 milestone Oct 9, 2020
@Bartunek
Copy link

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.

@goce-cz goce-cz modified the milestones: v2.0.0, v3.0.0 Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed weirdness Something is Confusing or unclear
Projects
None yet
Development

No branches or pull requests

5 participants