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

Investigate passing specific properties to React components #6

Open
bcherny opened this issue Jan 25, 2018 · 6 comments
Open

Investigate passing specific properties to React components #6

bcherny opened this issue Jan 25, 2018 · 6 comments

Comments

@bcherny
Copy link
Owner

bcherny commented Jan 25, 2018

Instead of passing the entire store.

Note: This will be hard to type safely.

Pros:

  • Let React do the diffing instead of doing it ourselves
  • Avoids the need to watch specific properties for changes and calling forceRender
  • Users can prevent renders with shouldComponentUpdate, and use other lifecycle methods

Alternative: Use an immutable data structure to back store, so the reference changes.

Cons:

  • Asymmetrical API for getting/setting (fixed by simplify the set/get of store, just like mobx #7):

    withStore()(({ foo, bar, set }) =>
     <div onClick={() => set('bar')(3)}>
       {foo}
       {bar}
     </div>
  • Precludes an API where we automatically infer what to watch based on what is used. We can't infer types from the arguments (unless there's a clever trick I haven't thought of), and passing store directly lets us do this

@bcherny bcherny self-assigned this Jan 25, 2018
@elderapo
Copy link

Wouldn't it be a good idea/possible (with type safety) to implement mechanism similar to reduxes mapStateToProps/mapDispatchToProps instead of passing 'foo', 'bar' to withStore?

@bcherny
Copy link
Owner Author

bcherny commented Jan 28, 2018

@elderapo By passing the store in as an argument, I'm hoping to avoid name collisions. I'm also hoping to avoid mapStateToProps/mapDispatchToProps for their extra complexity. Other APIs might include using a closure (but again, this may be too complex for users):

withStore(({ foo, bar, set }) =>
  ({ baz }) =>
    <div>..</div>
)

Work in progress is in this branch, if you're curious or have more thoughts.

@danawoodman
Copy link

Personally, this is the main turn-off for me with undux because IMHO it goes against React conventions. It feels wrong to call getter methods in my components. This also breaks the separation of concerns since now my component is bound to a method .get('foo') instead of just expecting a normal prop foo. Say I decide to stop using undux one day, now I have to go through all my components and replace the .get(...) calls.

If the trade off is to have a closure like you show, I'm 100% ok with that and think any beginner (eg people who know React well enough to pick a tool like undux) would be OK with it.

Just my $0.02! Really like what you're doing with undux!

@danawoodman
Copy link

Not sure if this is doable while keeping typechecking, but perhaps an API like this?

import React from 'react'
import { StoreProps, withStore } from './store'

type Props = {
  greeting: string
  markAsRead(): void
}

function Welcome<StoreProps && Props>({ greeting, markAsRead, messages, user }) {
    return (
      <div>
        <p>{greeting} {user.name}, you have {messages.length} messages!</p>
        <button onClick={markAsRead}>Clear new messages</button>
      <div>
    )
  }
}

// Option 1:
export default withStore(
  ({ messages, set, user }) => ({
    markAsRead() {
      set('messages', [])
    },
    messages,
    user,
  }),
  Welcome
)

// Option 2:
// export default withStore(
//   store => ({
//     markAsRead: () => store.set('messages', []),
//     messages: store.get('messages'),
//     user: store.get('user'),
//   }),
//   Welcome
// )

Usage:

<WelcomeMessage greeting="bonjour" />

Something like this is clear to the user while (hopefully) getting all the existing benefits of undux.

This may be a bit more verbose but it clearly separates the state management/access from the component which I think is desirable. This way the component stays unaware of the state management tool in use and can just expect specific props.

@bcherny
Copy link
Owner Author

bcherny commented Mar 4, 2018

Personally, this is the main turn-off for me with undux because IMHO it goes against React conventions. It feels wrong to call getter methods in my components. This also breaks the separation of concerns since now my component is bound to a method .get('foo') instead of just expecting a normal prop foo. Say I decide to stop using undux one day, now I have to go through all my components and replace the .get(...) calls.

@danawoodman Thanks for the feedback! Your proposed API looks reasonable, and your concerns are really fair. The one reason I haven't jumped on this issue is that I love how simple and easy to understand a single store prop is. Even if I've never worked with Redux (or even React) before, I've probably seen stores before, and the getter/setter API is really familiar. When I see store, I know what it is intuitively. With asymmetrical getters and setters, it's not so clear - I have to know about closures, spreads, and be familiar with how Undux works to use it (ie. I have to know that Undux spreads its store over a component's props).

Your Option 1 is nice, and is reminiscent of how Redux does it; I love how it helps decouple components from the underlying store, so as you said, you can swap out the store implementation without updating component code. The downside of this decoupling, I think, is indirection. It's no longer clear where props come from: this could be a good thing, or a bad thing; it's a good thing because it makes my component independent from the store API, and it's a bad thing because now it's hard to see where props come from at a glance at the component (as I mentioned, how have to understand a whole bunch of concepts to grok how props are getting there).

What do you think?

@emmanuelbuah
Copy link

emmanuelbuah commented Mar 13, 2018

Jumping in here. @danawoodman is spot on in terms of decoupling component implementation from the store per react style. @bcherny you are also right for I was drawn to undux because of its simplicity. I think the biggest gain undux provides is getting rid of all the action/reducer boilerplate that comes with redux. Anyone using undux will be using react and will have to understand the concept of HOC sooner rather than later. From my point of view, going with option 1 or 2 is still far better than redux with boilerplate. In short, I don't think going in any of the proposed options takes way the simplicity of the library compared to the status quo.

With that said, it also possible to offer the current api (passing the store into the component) as more or less a beginners level and the decoupled options per @danawoodman suggest for advanced usage. That should mitigate the risk of getting people up and running while providing a path for advanced usage as your application grows.

Personally, I think that will add complexity and might be best to roll with something along the lines of options 1 or 2 but you know best.

I'd also like to propose the 3rd option below which is a slight modification on option 1. This helps address the issue of what props come from where. ownProps depicts properties pass by react to the component.

export default withStore(
   // subscribe to changes to store value 'messages' similar to redux#mapStateToProps
   'messages',
   // similar to redux#mapDispatchToProps to update store state
   ({ set, ownProps }) => ({
     markAsRead() {
       set('messages', [])
     },
     ...ownProps
   }),
   Welcome
  )

@bcherny bcherny closed this as completed in bc90f3c Apr 6, 2018
@bcherny bcherny reopened this Apr 6, 2018
@bcherny bcherny removed their assignment Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants