subscribeWithSelector is unnecessarily complicated

[kettanaito]

Hi!

The current approach to subscribeWithSelector is unnecessarily complicated in a sense that it requires you to wrap your entire store for no apparent benefit (based on the current implementation). All it does is implements its own subscribe function on top of the store.subscribe.

The problem with this approach is that:

1. Store declaration becomes overtly complex (it's already cumbersome that one has to nest all the middlewares into each other when declaring the store).
2. It patches the default subscribe since subscribeWithSelector also creates the store. It mixed multiple unrelated concerns whereas it should only be focused on helping the user to subscribe to a particular slice of the state.

Proposed solution

The current implementation of the function can be rewritten to accept any store:

export function subscribeWithSelector<T, R>(
  store: StoreApi<T>,
  selector: (state: T) => R,
  updateFn: (slice: R, state: T, prevState: T) => void,
) {
  return store.subscribe((state, prevState) => {
    const prevSlice = selector(prevState)
    const nextSlice = selector(state)

    if (!Object.is(prevSlice, nextSlice)) {
      updateFn(nextSlice, state, prevState)
    }
  })
}

Granted, this is a version I'm using in my app and it's not 1-1 compatible with the current API. E.g. the options is missing, etc., which is extremely easy to add.

The benefit of this change is that now I can subscribe to any store anywhere I want to:

subscribeWithSelector(store, slice, handler)

I don't need to wrap my stores into this. It becomes what it is supposed to be: a way to subscribe to a slice of the state.

Let me know if this makes sense to you. I'm happy to open a pull request and help ship this. Thanks.

[dai-shi]

Thanks for raising it up.

Some history:

• I dropped the capability from the core and made this middleware for migration.
• I never recommended using this middleware for new projects as there should be easier ways accomplish the task.
• My plan is to deprecate this middleware and drop it completely.

Yes, I like your proposition. It's no longer middleware right? That's what I mean with the easier way.
So, it makes sense, but we don't host in this repo. Suggestions are:

• Create a new package in https://github.com/zustandjs
• And, deprecate subscribeWithSelector middleware recommending the new package.

[kettanaito]

Awesome! So it sounds we're aligned. I would be glad to turn my suggestion into a standalone package once I have a moment. Thanks for a quick response.

[dbritto-dev]

@kettanaito lmk if you need some help, I'd be happy to help to deprecated subscribeWithSelector middleware

[dai-shi]

Let's not discuss the exact migration steps for now.

That's no longer the case as there's no more replacement going on. The listener will be fired immediately always.

I think @kettanaito misunderstands what fireImmediately does.
It's basically this:

const fn = ...;
subscribe(fn);
fn(); // fire the function immediately after the subscription

I'm not sure if we should fireImmediately option to zustand-subscribe.
We can call the function by ourselves.
That said, fireImmediately was added based on the user request.
So, if we want to help transition from the middleware, we should either have the same option or a document to explain how we can accomplish the same behavior.
The same thing applies to equalityFn.

[dbritto-dev]

yeah, I'm down with you @kettanaito

[dbritto-dev]

@kettanaito I can create PR if you don't have time just ping me

[kettanaito]

Oh, I see! Yeah, I misunderstood the function. I can argue that fireImmediately is an invalid option to have here since the only way to fire a listener in an event-based system is for the event to occur. This sounds like a convenience method that's rather harmful and fails to teach the user about the basics of subscriptions. If you need fireImmediately, you really want getState + fire your own side effect.

I have nothing against equalityFn. @dbritto-dev I can open a pull request later next year. The same goes for code review. I'd be glad to review these changes and make sure we're keeping a top-notch API.

For example:

• equalityFn -> compare(prevValue, nextValue) (what it does vs what it is)
• fireImmediately I'd drop entirely and show an example of what to do instead.

[dai-shi]

Sounds good.

[dai-shi]

@kettanaito
Could you invite me in https://www.npmjs.com/package/zustand-subscribe just in case?

[kettanaito]

Absolutely! Invite sent.