xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
554 stars 25 forks source link

A way to extend arrayActions / objectActions ? #258

Closed mindreframer closed 3 years ago

mindreframer commented 3 years ago

Hey @xaviergonz, first: thank you for providing Mobx Keystone, I'm using it in my app and liking it much better than Mobx State Tree!

Currently I'm trying to figure out a possibility to extend generic array / object actions without tying it up to a particular model instance.

In concrete: arraySwap function is currently non-generic and requires a "host" model:

@modelAction arraySwap(array: any[], indexA: number, indexB: number) {
    if (indexA < 0 || indexB < 0 || indexA > array.length - 1 || indexB > array.length - 1) {
      return
    }
    var tmp1 = array[indexA]
    var tmp2 = array[indexB]
    array[indexA] = null
    array[indexB] = null
    array[indexA] = tmp2
    array[indexB] = tmp1
    return array
  }

When implemented with arrayActions primitives, it blows up my React.js app.

class ArrayUtils {
  static arraySwap(array: any[], indexA: number, indexB: number) {
    if (indexA < 0 || indexB < 0 || indexA > array.length - 1 || indexB > array.length - 1) {
      return
    }
    var tmp1 = array[indexA]
    var tmp2 = array[indexB]
    arrayActions.set(array, indexA, null)
    arrayActions.set(array, indexB, null)
    arrayActions.set(array, indexA, tmp2)
    arrayActions.set(array, indexA, tmp1)
    return array
  }
}

I see that arrayActions is using import { fnModel } from "./fnModel", and fnModel is not exposed for end users.

Are there any other options to implement further generic Array/Object actions, without using a mounted model instance?

As a short-term fix I could contribute a swap function on the ArrayActions module, yet this feels rather inflexible.

xaviergonz commented 3 years ago

my guess is that the react app is blowing because the operation is not being made in a batch, so it is trying to re-render while some elements are null

does it work if you make arraySwap a mobx action? e.g.

import {action} from "mobx"

const arraySwap = action((array: any[], indexA: number, indexB: number) => {
    if (indexA < 0 || indexB < 0 || indexA > array.length - 1 || indexB > array.length - 1) {
      return
    }
    var tmp1 = array[indexA]
    var tmp2 = array[indexB]
    arrayActions.set(array, indexA, null)
    arrayActions.set(array, indexB, null)
    arrayActions.set(array, indexA, tmp2)
    arrayActions.set(array, indexA, tmp1)
    return array
  });
mindreframer commented 3 years ago

@xaviergonz thanks for response!

Just tried it out wrapped into action, same. This brought me on an idea: using a fake item of similar shape instead of null. I get 4 undo entries in the undoMiddleware with all the intermediate steps + this makes this function non-generic. Wrapping it in action does not change anything.

Do you have any other ideas to try out?

xaviergonz commented 3 years ago

did you also try using undoManager.withGroup inside the action?

mindreframer commented 3 years ago

That is the issue... My undoManager is tied to a particular UI and would be not accessible in that generic module for array manipulations. I would like to be able to re-use it with different undoManagers in a transparent fashion.

Would exposing fnModel to the end user make it possible to implement user-specific generic array / object actions?

mindreframer commented 3 years ago

@xaviergonz I have tried exposing fnModel in standardActions/index.ts:

export * from "./arrayActions"
export * from "./fnModel"
export * from "./objectActions"

That's actually all that is required to solve this issue. Should I open a pull request with this 1-line change?

Or: what is speaking against exposing fnModel to the end-user?

mindreframer commented 3 years ago

I have opened up https://github.com/xaviergonz/mobx-keystone/pull/262 as suggestion to resolve this issue

mindreframer commented 3 years ago

Hey @xaviergonz, how can we move forward with this issue? I don't want to fork mobx-keystone because of this, but I am not sure, what other options I have... Exposing fnModel solves those issues completely. Please don't ignore this issue.

xaviergonz commented 3 years ago

I promise I'll take a look once I get a bit of time

mindreframer commented 3 years ago

Thanks!

xaviergonz commented 3 years ago

This will be out soon in v0.60.0

Standalone Actions

Sometimes you might need to define a "model" action but without an associated model. Say for example that you need an array swap method that needs the be processed by middlewares (undoable, etc.). One way to achieve this is to use standalone actions like this:

const arraySwap = standaloneAction(
  "myApp/arraySwap",
  <T>(array: T[], index1: number, index2: number): void => {
    if (index2 < index1) {
      ;[index1, index2] = [index2, index1]
    }
    // since a same node cannot be in two places at once we will remove
    // both then reinsert them
    const [v1] = array.splice(index1, 1)
    const [v2] = array.splice(index2 - 1, 1)
    array.splice(index1, 0, v2)
    array.splice(index2, 0, v1)
  }
)

Note the following pre-requisites apply to standalone actions:

does that solves your issue?

xaviergonz commented 3 years ago

out now

xaviergonz commented 3 years ago

Actually also added swap to array actions in 0.60.2 while at it

mindreframer commented 3 years ago

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 THANK YOU! This looks super-clean!

mindreframer commented 3 years ago

Just updated it in my codebase, works flawlessly. Thanks again for the extra effort to come up with such a clean solution.

Going to close this issue.