v0.3.0

[josepot]

After putting a lot of thought into this... These are the changes/improvements that I would like to suggest for version 0.3.0:

• As we have already discussed offline, it's just better to get rid of the unsubscribeGraceTime parameter.
• I've made a few improvements on the logic that handles the async errors on Suspense
• The most important change: currently the factory observable cleans-up its cache once the observable has lost all its subscribers. That's very annoying. So, I thought about a lot of options: having a flag for persisting it, passing a duration observable, etc, etc... And then I realized that in fact there is a much simpler and logical approach: only remove it if it has completed by the time that the refcount goes down to zero. There are some new tests for that functionality. I really think that this is the way to go.

[codecov[bot]]

Codecov Report

Merging #104 into main will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #104   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines          371       368    -3     
  Branches        41        38    -3     
=========================================
- Hits           371       368    -3     

Impacted Files	Coverage Δ
packages/core/src/bind/index.ts	100.00% <ø> (ø)
packages/core/src/bind/connectFactoryObservable.ts	100.00% <100.00%> (ø)
packages/core/src/bind/connectObservable.ts	100.00% <100.00%> (ø)
packages/core/src/internal/react-enhancer.ts	100.00% <100.00%> (ø)
packages/core/src/internal/useObservable.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 74ee837...078d1bc. Read the comment docs.

[voliva]

It's very impressive, but I think this route has a dead end. I can't think of a way to make this test work:

it("dismisses the observable if it has completed without subscribers", async () => {
  let nInitCount = 0
  const observable$ = defer(() => {
    nInitCount += 1
    return from([1, 2, 3, 4, 5])
  })

  const complete$ = new Subject()
  const [useLatestNumber] = bind((id: number) =>
    concat(observable$, of(id), complete$).pipe(shareReplay(1)),
  )
  const { unmount } = renderHook(() => useLatestNumber(6))
  expect(nInitCount).toBe(1)

  unmount()
  await wait(270)

  complete$.complete()

  await wait(270)

  renderHook(() => useLatestNumber(6))
  expect(nInitCount).toBe(2) // Fails
})

Because in order to receive that complete, you need to be subscribed to it. But if you keep an active subscription, then we're breaking the majority of the streams that are used with bind (i.e. the ones that don't use shareReplay). It feels like the only way would be to see if that stream has unsubscribed from its sources, but we don't have control for that.

It's worth noting that from that point onwards it will work: When refcount hits 0, the stream will be cleaned of... (but maybe too late? - thinking out loud here)

If we decide to drop support for shareReplay in bind factory observables, we can always add a rule in eslint (and think of alternatives for a generic case).

[josepot]

Yep, you are right @voliva . I think that we should leave the factory bind as it is right now. The reason being that the practice that we should be promoting is that an observable returned by a factory bind should backed up by a "top level" observable. For instance:

const todos$ = todoActions$.pipe(
  split(
    event => event.payload.id,
    (event$, id) => event.pipe(
      takeWhile(event => event.type !== 'delete'),
      scan(
        (state, action) => {
          switch (action.type) {
            case "add":
            case "edit":
              return { ...state, text: action.payload.text }
            case "toggle":
              return { ...state, done: !state.done},
            default:
              return state
          }
        },
        { id, text: "", done: false }
      )
    )
  ),
  collect()
)

const [useTodosIds] = bind(todos$.pipe(map(todosMap => [...todosMap.keys()]))

const [useTodo] = bind((id: number) => todos$.pipe(
  take(1),
  switchMap(todosMap => todosMap.get(id))
))

Or, if todos$ was enhanced with collectValues, instead of collect, then useTodo would look like this:

const [useTodo] = bind((id: number) => todos$.pipe(
  map(todosMap => todosMap.get(id))
))

We should promote this practice even for things that come from React (rather than coming for an observable):

const itemIdsRequested$ = new Subject<number>()

const items$ = itemIdsRequested$.pipe(
  split(
    id => id,
    id$ => id$.pipe(switchMap(getItem))
  ),
  collectValues()
)

const [useItem] = bind((id: number) => merge(
  items$.pipe(
    filter(itemsMap => itemsMap.has(id)),
    map(itemsMap => itemsMap.get(id))
  ),
  defer(() => {
    itemIdsRequested$.next(id);
    return EMPTY;
  }),
))

const Item: React.FC<{id: number}> = ({ id }) => {
  const item = useItem(id);
  return <div>{item.name}</div>
}

const Items: React.FC: <{itemIds: number[]}> = ({ itemIds }) => {
  useSubscription(items$);
  return itemIds.map(id => (
    <Suspense key={id} fallback={<div>'Loading...'</div>}>
      <Item id={id} />
    </Suspense>
  )) 
}

Admittedly, it's not very ergonomic, but it gives the user full-control on how to cache the values/streams.