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.

Sign up for GitHub

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

Jump to bottom

Operator to create events for updates from external sources #729

Open
AlexandrHoroshih opened this issue Jun 16, 2022 · 3 comments · May be fixed by #796
Open

Operator to create events for updates from external sources #729

AlexandrHoroshih opened this issue Jun 16, 2022 · 3 comments · May be fixed by #796
Labels
RFC Request for Comments

Comments

@AlexandrHoroshih
Copy link
Member

AlexandrHoroshih commented Jun 16, 2022 •
edited by sergeysova

There is a common pattern to connect external event source to effector with scope support:

const connectSocketFx = attach({
  source: $socket,
  effect(socket) {
    socket.on("message", scopeBind(messageReceived))
  }
})

More details and problems with this solution here: #666

I suggest to add new api to cover all cases with external observables - with full Fork API support

function fromReactiveSource<Payload, S>(config: {
  source?: Store<S>,
  setupOn: Clock<any>, // it is essential for Fork API support to have explicit unit to setup subscription in correct scope
  setup: (scopedTrigger: (p: Payload) => Payload, sourceData?: S) => void;
}): Event<Payload>

with usage like this:

const $history = createStore(createBrowserHistory())

const historyUpdated = fromReactiveSource<HistoryUpdate>({
  source: $history,
  setupOn: [appStarted, ...],
  setup: (scopedTrigger, history) => {
    history.listen(scopedTrigger)
  }
})

// or with target
fromReactiveSource<HistoryUpdate>({
  source: $history,
  setupOn: [appStarted, ...],
  setup: (scopedTrigger, history) => {
    history.listen(scopedTrigger)
  },
  target: [historyUpdated, ...]
})

I think, it will allow to cover all cases with external subscriptions + scope support, since the latter requires explicit trigger to setup subscription, so its correctly scope bounded

Also, i think, it might be useful to have an option to provide destruction function and trigger:

const $history = createStore(createBrowserHistory())

const historyUpdated = fromReactiveSource<HistoryUpdate>({
  source: $history,
  setupOn: [appStarted, ...],
  setup: (scopedTrigger, history) => {
    const unlisten = history.listen(scopedTrigger);

    return () => unlisten()
  },
  destroyOn: appUnmounted
})

This will allow to avoid memory leaks during SSR usage, where user would need to create separate subscription for every scope (i.e. per request) and also destroy this subscription, when request is handled and scope should be thrown away

Originally posted by @AlexandrHoroshih in #490 (comment)
@AlexandrHoroshih AlexandrHoroshih added the RFC Request for Comments label Jun 16, 2022
@AlexandrHoroshih
Copy link
Member Author

AlexandrHoroshih commented Jun 16, 2022 •
edited

Also maybe it is better to extend Scope like this:

scope.destroy()

to explicitly call reactive-source destroyers to drop all scope-bound subscriptions and additional links

@AlexandrHoroshih
Copy link
Member Author

Also we already have an operator for integration with external sources:
https://effector.dev/docs/api/effector/fromObservable

So, looks like this RFC is about extending api of fromObservable

@AlexandrHoroshih
Copy link
Member Author 

const event = fromObservable({
 source: $history,
 clock: appStarted,
 stop: appStopped,
 setup: (scopedTrigger, history) => {
   const unlisten = history.listen(scopedTrigger);

    return () => unlisten()
 }
})

const event = fromObservable({
 clock: appStarted,
 stop: appStopped,
 setup:  rxjs.interval(1000),
})

@AlexandrHoroshih AlexandrHoroshih linked a pull request Sep 30, 2022 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC Request for Comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fromObservable API extension with Fork API support effector/effector
1 participant
@AlexandrHoroshih