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

subscribe API with state as an argument #303

Closed
staltz opened this issue Jul 23, 2015 · 16 comments
Closed

subscribe API with state as an argument #303

staltz opened this issue Jul 23, 2015 · 16 comments

Comments

@staltz
Copy link

@staltz staltz commented Jul 23, 2015

Why

store.subscribe(() => console.log(store.getState()));

and not

store.subscribe(state => console.log(state));

?

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 23, 2015

Yeah I should probably do that.

I wanted to bike shed a little — maybe worth also adding previousState as a second parameter so consumer can compare reference without keeping the local state copy?

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 23, 2015

e.g.

store.subscribe((state, previousState) => {
  if (selectStuffIWant(state) !== selectStuffIWant(previousState)) {
    doSomething(state);
  }
});
@gaearon gaearon added the discussion label Jul 23, 2015
@gaearon gaearon added this to the 1.0 milestone Jul 23, 2015
@staltz
Copy link
Author

@staltz staltz commented Jul 23, 2015

previousState as a second parameter so consumer can compare reference without keeping the local state copy?

Just a reminder how this is where Rx shines:

store.subscribe(state => console.log(state));

or

store.pairwise().subscribe((previousState, state) => {
  console.log(previousState);
  console.log(state);
});
@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 23, 2015

Haha, I know. :-)

This is why I didn't bother to pass it into the callback initially. People who want to work directly with subscribe API will probably want to wrap it into an observable anyway..

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 23, 2015

In fact Redux createStore API can probably be implemented with two or three lines in Rx.

My intention was to extract a subset that can be used by people who don't want Rx dependency and want some opinionated wiring defaults (one root reducer, actions as plain objects) to get some benefits (logging, record/replay, time travel) “by default”.

@acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 23, 2015

Best to keep this a low level API. Passing current state to subscriber seems reasonable, though. For anything more complicated, it takes two lines to convert to observable https://github.com/acdlite/redux-rx/blob/master/src/observableFromStore.js

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 23, 2015

OK I'll accept a PR passing the parameter. One tiny requirement is to make sure it works correctly when Redux store is instrumented by Redux DevTools. I have a bad feeling that every store extension that “lifts” getState will also have to take special care of this parameter. Which is not fun and feels like the price you pay for making low-level APIs more consumer friendly.

@acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 23, 2015

Ah yes that's an excellent point. In that case, I'd say leave it as-is.

@iclanzan
Copy link

@iclanzan iclanzan commented Jul 24, 2015

Please pass both current and previous state to listeners. I really don’t want to have to keep a reference to the previous state outside the listener and cannot afford to include Rx into my project.

@gaydenko
Copy link

@gaydenko gaydenko commented Jul 27, 2015

I have asked in Slack channel exactly the same question as the one at the beginning of this issue. My confusing is: why a listener is forced to know about store (or anything else with getState() method) at all?

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 27, 2015

The store API is meant to be extensible. This is why it needs to be as orthogonal as possible. The methods shouldn't duplicate functionality because otherwise creating extensions to it like Redux DevTools becomes harder. If extensions want to do something to the state before passing it to the consumer, they'll now have to do it in two different places (three if we add two parameters!) which is error prone.

The solution is simple: don't use subscribe directly! It's a low level API. If you want it to behave as an Observable, write a function that turns it into an observable! It isn't hard to do:

function toObservable(store) {
  return {
    subscribe({ onNext }) {
      let dispose = store.subscribe(() => onNext(store.getState()));
      onNext(store.getState());
      return { dispose };
    }
  }
}

If you don't want to use RX and prefer a callback with previous and next value, again, easy to do. You can even do selecting as part of it:

function observeStore(store, select, onChange) {
  let currentState;

  function handleChange() {
    let nextState = select(store.getState());
    if (nextState !== currentState) {
      currentState = nextState;
      onChange(currentState);
    }
  }

  let unsubscribe = store.subscribe(handleChange);
  handleChange();
  return unsubscribe;
}

We can provide either as a utility inside Redux, if we can converge on the API people want.

What we can't do is make Store interface less low-level and duplicating functionality between methods, because then building extensions on top of it becomes way harder.

@staltz
Copy link
Author

@staltz staltz commented Jul 27, 2015

The solution is simple: don't use subscribe directly! It's a low level API.

👍 That's enough clarifications to me, I'd be ready to close this issue.

@gaydenko
Copy link

@gaydenko gaydenko commented Jul 27, 2015

@gaearon , thanks for the elaborate clarification!

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 27, 2015

I'll keep this open to make sure I don't forget to add this to the docs.

@gaearon gaearon added the docs label Jul 27, 2015
@phaistonian
Copy link

@phaistonian phaistonian commented Jul 29, 2015

Related:

How about:

store.subscribe(
  state => console.log(state),
  (previousState, state) => previousState.something !== state.something
);

In other words, being able to pass an optional method to store.subscribe that will provide a check against previous state.

That's the usual scenario anyway.

This will probably make things a little less dirty in the long run.

@gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 30, 2015

Closing for the reasons described here: #303 (comment). This is documented:

It is a low-level API. Most likely, instead of using it directly, you’ll use React (or other) bindings. If you feel that the callback needs to be invoked with the current state, you might want to convert the store to an Observable or write a custom observeStore utility instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.