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

Usage with immer #45

Closed
nmklotas opened this issue Apr 8, 2019 · 11 comments
Closed

Usage with immer #45

nmklotas opened this issue Apr 8, 2019 · 11 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@nmklotas
Copy link

nmklotas commented Apr 8, 2019

Hello,

Wonderful library!

Do you have plans to integrate immer (https://github.com/mweststrate/immer)?
That would make writing reducers very easy

I'm trying to create my own version of createReducer that uses immer but without luck, maybe you know how to use your library with immer?

Thanks

@the-dr-lazy the-dr-lazy added enhancement New feature or request question Further information is requested labels Apr 8, 2019
@the-dr-lazy
Copy link
Owner

Hi @nmklotas

Do you have plans to integrate immer (https://github.com/mweststrate/immer)?

Yes, there are some workarounds to make internal integration of Deox with immer but I'm not sure about them for now.

I'm trying to create my own version of createReducer that uses immer but without luck, maybe you know how to use your library with immer?

A simple example of using immer with Deox could be like below:

import produce from 'immer'

const todos = createReducer(defaultState, handleAction => [
  handleAction(addTodo, (state, { payload }) =>
    produce(state, draft => {
      draft.push(payload)
    })
  ),
])

For a reducer factory that uses immer you can do as follows for now:

import produce from 'immer'

function createImmerReducer(defaultState, handlerMapsCreator) {
  const handlerMap = merge(...handlerMapsCreator(createHandlerMap))

  return (state, action) => {
    const handler = handlerMap[action.type]

    return handler ? produce(state, draft => handler(draft, action)) : state
  }
}

@nmklotas
Copy link
Author

Thanks for the response!

To createImmerReducer I would need additional exports from your package, otherwise TypeScript will would not find them:

can you add these exports?

export { HandlerMap, CreateHandlerMap, createHandlerMap } from './create-handler-map';
export { merge } from './utils';

@the-dr-lazy
Copy link
Owner

Some of those API may change without deprecations or breaking changes. So you can try submodule imports at your own risk in the following way:

import { HandlerMap, CreateHandlerMap, createHandlerMap } from 'deox/create-handler-map'
import { merge } from 'deox/utils'

@the-dr-lazy
Copy link
Owner

I will close the issue for housekeeping purposes. Feel free to re-open it if needed.

@the-dr-lazy
Copy link
Owner

the-dr-lazy commented May 19, 2019

I have an idea but I need more community feedback about it.

The idea is that export an API from Deox for extensions which can add more functionality to handleAction function.

For example in immer it could be like below:

import { createReducer } from 'deox'

const userReducer = createReducer(defaultState, handleAction => [
  handleAction.immer(updateUser, (draft, { payload }) => {
    state.username = payload.username || draft.username
    state.isAdmin = payload.isAdmin || draft.isAdmin
  })
])

Personally, I don't use immer. So @nmklotas @Haaxor1689 @LouizFC @rlesniak if you guys have experience on it, it would be great to help us here.

@the-dr-lazy the-dr-lazy reopened this May 19, 2019
@the-dr-lazy the-dr-lazy added the help wanted Extra attention is needed label May 19, 2019
@LouizFC
Copy link
Contributor

LouizFC commented May 20, 2019

Looks good, but I think the naming should be stateDraft or draftState

Also, in immer you can't replace the entire state object, like

produce(state, (draft) => {
  draft = newState;
})

So it should be taken into account

@the-dr-lazy
Copy link
Owner

the-dr-lazy commented May 20, 2019

Also, in immer you can't replace the entire state object.
So it should be taken into account

Which aspect of this pitfall should we consider in Deox?

@LouizFC
Copy link
Contributor

LouizFC commented May 20, 2019

I don't think it is something that should be addressed in code. Just documenting the usage patterns should be enough.

Replacing the entire state tree is rare, and happens mostly on "sub-reducers".

If someone need to replace the entire state, then instead of using handleAction.immer, they can use the standard handleAction.

Something like:

import { createReducer } from 'deox'

const userReducer = createReducer(defaultState, handleAction => [
  handleAction.immer(updateUser, (draft, { payload }) => {
    draft.username = payload.username || state.username
    draft.isAdmin = payload.isAdmin || state.isAdmin
  }),
  handleAction(replaceUserData, (state, { payload } => payload)
])

Or a similar pattern

Edit: Also, in immer you can't create a "Draft of a Draft" (at least you couldn't, I will try to test it later to see if it stays true), so having the original state object is good.

The handler could look something like:

 handleAction.immer(updateUser, (state, draft, { payload }) => {
    draft.username = payload.username || state.username
    draft.isAdmin = payload.isAdmin || state.isAdmin
  })

Edit 2: another option is using the mechanism that immer uses for this cases, read it here and here

@the-dr-lazy
Copy link
Owner

Also, in immer you can't create a "Draft of a Draft"

Is it a pattern in immer to create a draft from a draft?

so having the original state object is good.

Can you explain more about the benefit(s) or use case(s) in favor of this extra argument?

@LouizFC
Copy link
Contributor

LouizFC commented May 21, 2019

Everything that I will explain can probably be found and better explained on Immer README, but I will try to sum up some things

Can you explain more about the benefit(s) or use case(s) in favor of this extra argument?

Immer drafts uses Proxy behind the scenes on compatible browsers, and fallsback to a "polyfilled" version on non-compatible browsers.

That means that these drafts comes with costs, both on getting or setting a property.

These perfomance costs are not exorbitant, so it is not a "do or die" situation.

It is recommended to use the original state for accessing properties instead of the draftState, the more nested the state is, more costly access becomes.

Also, it is recommended not to "leak" the draft object into the application, because it will throw an error if you try to do anything with it after produce is resolved.

Making functions to make reusable "mutations" is ok, but using draft as if it was the original object is bug prone.

Is it a pattern in immer to create a draft from a draft?

No, I just mentioned it because I had a case in mind but forgot to share more context.

Sometimes you need to call a reducer inside another reducer, like so:

handle(resolveQuery, (state, action) =>
    produce(state, (draft) => {
        draft.destination = findNearestDestination(state, action.payload);
        draft.query = QueryReducer(state.query, action);
    })
  ),

I can't tell if passing a draft to findNearestDestination or QueryReducer would be bug prone in the long run, but following the recommendations in the README, it looks like a bad pratice, but I could be wrong.

@the-dr-lazy
Copy link
Owner

As this issue should handle by plugins, I'm gonna close it until plugins system become into action. Feel free to reopen the issue if it's needed.

@the-dr-lazy the-dr-lazy removed the help wanted Extra attention is needed label Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants