jedireza / aqua

:bulb: A website and user system starter
https://jedireza.github.io/aqua/
MIT License
1.38k stars 356 forks source link

Why not use connect() to bind stores #235

Closed jacogreyling closed 7 years ago

jacogreyling commented 7 years ago

Just curious,

any particular reason why you've not opted for the react-redux module in your code? All the documentation around shows how to connect a React component to a Redux store using connect().

As per the official documentation: "If you really need to, you can manually pass store as a prop to every connect()ed component, but we only recommend to do this for stubbing store in unit tests, or in non-fully-React codebases. Normally, you should just use ."

This is not criticism, I'm just curious as to the rationale behind it?

Thx!

jedireza commented 7 years ago

Thanks for asking. I don't remember my original thoughts for not using react-redux. Maybe it was to keep things simple (one less module/dependency).

After skimming some of the docs...

...there are a couple things that stick out to me:

const VisibleTodoList = connect(
  mapStateToProps,
  mapDispatchToProps
)(TodoList)

Maybe I haven't given it enough time to grow on me. The docs do mention:

Technically you could write the container components by hand using store.subscribe(). We don't advise you to do this because React Redux makes many performance optimizations that are hard to do by hand.

So I don't doubt there are some upsides to using it. I'd be interested in the effect using react-redux would have to the Aqua code base. I don't plan on spending time looking into this, but if someone wants to make a proposal and show some examples I'd be happy to work through it more then.

jacogreyling commented 7 years ago

Thank you as always for your comprehensive response. I agree I also don't like the notation used with overloaded methods (ok so I come from a Java background). As you mention there seems to be upsides though that needs to be explored further.

Just looking at the connect() function I can see the following:

      componentDidMount() {
        // it remembers to subscribe to the store so it doesn't miss updates
        this.unsubscribe = store.subscribe(this.handleChange.bind(this))
      }

      componentWillUnmount() {
        // and unsubscribe later
        this.unsubscribe()
      }

      handleChange() {
        // and whenever the store state changes, it re-renders.
        this.forceUpdate()
      }

Comparing that to your code, it's very similar. The only difference is on an update they call the React forceUpdate() API which will in turn calls render(). (They handle getState() in render())

acouch commented 7 years ago

FWIW as someone new to react I appreciated seeing another way of implementing the connect() paradigm and agree that the connect API syntax is confusing.