drcmda / react-contextual

🚀 react-contextual is a small (less than 1KB) helper around React 16s new context api
MIT License
642 stars 22 forks source link

Question: Can I define the store using plain-old-javascript-object? #20

Closed songguoqiang closed 6 years ago

songguoqiang commented 6 years ago

Wth the current API, when I define the store, I need to specify "initialState" and "action" field. Is it possible (and better?) to support defining the store using plain-old-javascript-object format? e.g.

import { Provider, Subscribe } from 'react-contextual'

const counter = {
    count: 0,
    up: () => state => ({ count: state.count + 1 }),
    down: () => state => ({ count: state.count - 1 })
}

const user = {
    name: null,
    setName: name => ({ name })
}

const store = {
    counter,
    user
 }

const App = () => (
    <Provider {...store}>
        <Subscribe>
            {props => (
                <div>
                    <h1>{props.counter.count}</h1>
                    <button onClick={props.counter.up}>Up</button>
                    <button onClick={props.counter.down}>Down</button>
                </div>
            )}
        </Subscribe>
    </Provider>
)

If we encapsulate the state and the related action into one single object, that's more aligned with the standard object-oriented design approach, isn't it?

drcmda commented 6 years ago

@songguoqiang Actually that's a lovely idea! Could i interest you in a PR? 🤩Otherwise i will give this a go, but i'm pretty busy these days at work.

drcmda commented 6 years ago

The only downside i can see would be that previously you could have put random non-reactive globals into the store that would be available to subscribers. For instance i am doing this in one of our apps at work:

const store = createStore({
    initialState: { ... },
    // Application actions ...
    actions: { ... },
    // Server actions,
    writeReport: (array, output) => backend.writeReport(array, output),
    openFiles: (...args) => backend.openFiles(...args),
    getFiles: (...args) => backend.getFiles(...args),
    // Selectors,
    selectors: {
        usageArea: createSelector(
            [state => state.session.cpu, state => state.session.tasks],
            (cpu, tasks) => (area = [...area, { cpu, tasks }].slice(1).map((item, index) => ({ ...item, index }))),
        ),
        ...

@subscribe(store, store => ({ data: store.selectors.usageArea(store), tasks: Object.keys(store.tasks).length }))
export default class Chart extends React.PureComponent {

Server actions are some globally available functions, and selectors is using reselect. The actions would be overwritten, because react-contextual has to bind them, and the selectors thing would be treated as a namespace.

songguoqiang commented 6 years ago

@drcmda Let me take a look at the codes this weekend and give it more thoughts.

drcmda commented 6 years ago

Maybe as an optional arg/prop

const store = createStore({ counter, user }, { selectors, randomStuff })
const store = { counter, user }
<Provider {...store} global={{ selectors, randomStuff }}>

?

songguoqiang commented 6 years ago

A few more thoughts from me:

  1. There could be another possible API
const store = { counter, user, selectors, randomStuff }
<Provider {...store} }>

With this API, the Provider implementation needs to apply some magic:

There are pros and cons of each choice of the new API. Maybe we should give it more discussion before we make the change.

  1. These API changes are not backward compatible.

  2. I had a look at the source code. To make the new API works, there will be quite some changes in store.js. One major decision is what state do we store in the Provider component. With the current API (and implementation) the state object of the Provider object is shallow, like

this.state = {
   name: xxx,
   count: xxx
}

If we change the state object in Provider component to a nested data structure, e.g.

this.sate = {counter, user}

Then there will be impacts on how people write the actions to update those state (e.g. they have to use 'state.user.name' instead of 'state.name'.

It could also be tricky on how to merge the new state back to the state of Provider, e.g. considering the action implementation below:

const counter = {
    count: 0,
    up: () => state => ({ count: state.counter.count + 1 }),
    down: () => state => ({ count: state.counter.count - 1 })
}

The second choice is, if we decide to keep the state of Provider object shallow, then we need to implement some mapping from the store object

const store = { counter, user, selectors, randomStuff }

to the state of Provider.

drcmda commented 6 years ago

@songguoqiang As for merging state back in, i think that could be taken care of quite easily since these functions are bound in store.js, they should know where to apply updates and that it always affects their own namespace.

As for globals, i thought about it. I doubt anyone except me is using it. Even if we cast it out completely, they're just globals anyway, they could live in a separate module just fine. So if it makes it cleaner, we can rip globals out completely.

BTW, If you like, i'd gladly share authorship - i like your ideas a lot and if you want to make a branch to implement namespace (without globals), that would be great.

songguoqiang commented 6 years ago

Sorry for late reply. My work is a bit heavy these few days. I will fork this repository and play with the idea this weekend.

songguoqiang commented 6 years ago

@drcmda What coding style/standard do you follow in this project?

drcmda commented 6 years ago

For now, none, could you copy the settings from https://github.com/drcmda/react-spring ?

"scripts": {
    "prebuild": "rimraf dist",
    "build": "rollup -c",
    "prepare": "npm run build",
    "test": "echo will do",
    "precommit": "lint-staged"
  },
  "prettier": {
    "semi": false,
    "trailingComma": "es5",
    "singleQuote": true,
    "jsxBracketSameLine": true,
    "tabWidth": 2,
    "printWidth": 80
  },
  "lint-staged": {
    "*.{js,md}": [
      "prettier --write",
      "git add"
    ]
  },

Or if you have specific wishes, it's fine to add them ...

songguoqiang commented 6 years ago

After playing with this for a while, I would like to propose the following API.

To use the built-in ProviderContext

const store = {
    count: 0,
    up: () => state => ({ count: state.count + 1 }),
    down: () => state => ({ count: state.count - 1 }),
}

const App = () => (
    <Provider {...store}>
        <Subscribe>
            {props => (
                <div>
                    <h1>{props.count}</h1>
                    <button onClick={props.up}>Up</button>
                    <button onClick={props.down}>Down</button>
                </div>
            )}
        </Subscribe>
    </Provider>
)

To use external store

const externalStore = createStore({
    text: 'Hello',
    setText: text => ({ text })
})

const App = () => (
    <Provider store={store}>
        <Subscribe to={store}>
            {props => <div>{props.text}</div>}
        </Subscribe>
    </Provider>
)

Compared with the current API, there are two changes: (1) we don't need to specify "initialState" and "action" anymore. (2) we don't support the global functions in the store anymore.

Each store contains just one javascript object, which has some states and the functions to update those states. In this way, the store concept is similar to the container concept in the unstated.

I gave up the idea to put multiple objects in one store, because that brings too many changes to existing API and unnecessary complexity in the implementation. If someone has multiple independent states (e.g. current user in session, and the error messages from some backend API, etc), then they can create multiple stores, and the consumers can subscribe to the store they are intersted in.

All the other features such as the subscribe function should work without any change.

I tested this idea with my fork of the project, you can find my changes at https://github.com/songguoqiang/react-contextual.

drcmda commented 6 years ago

I like it, please add a pr, then i suppose tests have to be adapted and we publish it as a major. If you want to share authorship, need direct gh access - like i said, i'm totally open for it. 😃

songguoqiang commented 6 years ago

Sorry for late reply. I only get some time to work on this during weekends.

I just raised a PR. For now, I will make some contributions via PR first. If I have more frequent change requests, I will apply for direct access =)