byte-fe / react-model

The next generation state management library for React
236 stars 23 forks source link

Better Api Design #85

Closed ArrayZoneYour closed 4 years ago

ArrayZoneYour commented 5 years ago

Updated 2019-3-39

I have a:

  1. [x] Idea:

    • [x] What problem would it solve for you? 🐛

      These days, I review the model layer code in our teams and found something awkward. Form.ts

        const model = {
            // ...
            actions: {
                  updateA: (s, _, payload) => { ... },
                  updateB: (s, _, payload) => { ... },
                  updateC: (s, _, payload) => { ... },
                  updateD: (s, _, payload) => { ... }
             }
        }

      🤦‍♀️actions is useless at the most cases, and actions is also the return value of Model now, so why should we write _ again and again?

      The better we I thought:

      v2.x (no breaking changes, only new usage added, log warning for old usage)

      Form.ts

        import { actions } from './index.model'
        const model = Model({
            // ...
            actions: {
                  updateA: (payload, ctx) => { ... },
                  updateB: (payload, ctx) => {
                       const { state, actions } = ctx
                       actions.Form.updateA()
                       // ...
                  },
                  updateC: (payload, ctx) => { ... },
                  updateD: (payload, ctx) => { ... }
             }
        })
       const subscription = model.subscribe('updateA', () => { ... })
       setTimeout(() => { subscription.unsubscribe() })
       export default model

      v3.x

      Drop old object usage.

    • [x] Do you think others will benefit from this change as well and it should in core package? 💡 Yes

    • [x] Are you willing to (attempt) a PR yourself? ⚔ Yes

What do you think of it? Vote by yourself!

leecade commented 5 years ago

payload should be the first param?

actions: {
           updateA: (payload, state, actions) => { ... },
           updateA: (_, state, actions) => { ... },
           updateA: (name) => ({ name }),
ArrayZoneYour commented 5 years ago

payload should be the first param?

actions: {
           updateA: (payload, state, actions) => { ... },
           updateA: (_, state, actions) => { ... },
           updateA: (name) => ({ name }),

Good idea, I will consider it. But some problems remain here if payload be the first params.

  1. the order of (state, payload) is the same as Redux, easier to migrate.
  2. Our users use actions to fetch the api data in the most cases, actions must be async function. The TypeScript's bug force us to typing the return function input params like:
increment: async (s) => {
  // async function return produce need define type manually.
  const response = await api.fetch(...)
  return (state: typeof s) => {
    state.data += response.data || {}
  }
}

But I don't know when it will be fixed. TypeScript has much bugs to fix.💔

Extra:

actions should import from Model in the future, It won't be the params of action

import { Model } from 'react-model'
import Home from '../model/home.model'
import Shared from '../model/shared.model'

const models = { Home, Shared }

// old usage
// export const { getInitialState, useStore, getState, getActions } = Model(models)
export const { getInitialState, useStore, getState, actions } = Model(models)
leecade commented 5 years ago

useStore => useModel