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

simplify the set/get of store, just like mobx #7

Open
leecade opened this issue Jan 27, 2018 · 5 comments
Open

simplify the set/get of store, just like mobx #7

leecade opened this issue Jan 27, 2018 · 5 comments

Comments

@leecade
Copy link

leecade commented Jan 27, 2018

store.get('addTodoTitle') => store.addTodoTitle
store.set('addTodoTitle') => store.addTodoTitle = ''

@bcherny bcherny self-assigned this Jan 28, 2018
@bcherny
Copy link
Owner

bcherny commented Jan 28, 2018

This is an interesting idea. I went ahead and implemented it as an experiment in this PR.

Three downsides of this sort of setter syntax:

  1. We can't use curried setters, which are a convenient feature that works nicely with React APIs https://github.com/bcherny/undux#partial-application-all-the-way-through
  2. Setting a value now looks really different than React's setState API. The similarity between .set and .setState is intentional, and I think helps people understand what they're doing.
  3. It's hard to understand at a glance that you're updating an Undux store (it just looks like you're setting a property on a prop, which is bad). It's magic, which might be unnecessary.

All of these points may be reasonable compromises for the sake of simplicity. So the API would look like:

withStore('a')(store =>
  <div>a is {store.a}</div>
  <button onClick={() => store.a++}>Add 1 to a</button>
)

@hackerxian
Copy link

Do you consider this? Maybe better understand

store.set('number', 1) ;
store.set({number: 1}) ;

@bcherny
Copy link
Owner

bcherny commented Sep 3, 2018

@hackerxian I did, and thanks for bringing it up! Both syntaxes are great (in fact we could easily support all 3), but there are a few small downsides, I think:

  1. Having >1 way to do something makes me more uncertain how to do that thing. If I see 3 different syntaxes floating around, which is the right one? Is one a legacy syntax? Are there differences between them?
  2. Using Undux in a really large codebase, a lot of people don't read docs and instead grep for usages. Assuming one form (eg. store.set(k)(v)) is the most popular in that codebase, people will use it anyway. When someone else comes along and uses a different form (eg. store.set(k, v)), it might confuse people in code review (back to (1) again).
  3. Speaking of grepping for usages, if you want to see every place where your key 'userList' gets set today, you just grep for store.set('userList') and you'll get all the usages. If there are 3 forms, you'll have to grep for those too.
  4. The store.set(k)(v) form encourages people to think in terms of partial application, which gives more concise, more readable code (onChange={store.set(k)}).

Admittedly, these are pretty minor downsides. I think the guiding principle is: 1 way to do things, not more.

@zhy0216
Copy link

zhy0216 commented Apr 25, 2019

can we automatically generate getter/setter, but not sure if we still can get advantage of compile errors.
store.get('addTodoTitle') => store.getAddTodoTitle
store.set('addTodoTitle') => store.setAddTodoTitle

@bcherny
Copy link
Owner

bcherny commented Apr 25, 2019

@zhy0216 There's no way to do that safely, unless we introduce a codegen script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants