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

Add configurable merge strategies #5

Open
agilgur5 opened this issue Jul 7, 2019 · 3 comments
Open

Add configurable merge strategies #5

agilgur5 opened this issue Jul 7, 2019 · 3 comments
Labels
kind: feature New feature or request solution: workaround available There is a workaround available for this

Comments

@agilgur5
Copy link
Owner

agilgur5 commented Jul 7, 2019

Per my comments in #1 (comment), should support something like redux-persist's State Reconcilers as merge strategies. This is basically just to handle state created in .create(...), unless persist is called after some actions have been performed (which wouldn't be the standard usage, but is possible 🤷‍♂).

applySnapshot is equivalent to hardSet but with the model's defaults added. Users should be able to configure if they want a different merge strategy like a shallow merge (autoMergeLevel1).

@agilgur5
Copy link
Owner Author

agilgur5 commented Nov 9, 2019

Just noting that this and redux-persist's State Reconcilers are both only for initial state. This should be made very explicit in any docs for this. Merging existing, in-memory data is out-of-scope and should be done in application code / actions, not in mst-persist as it's in-memory and not related to persisting to disk.

That being said, we could expose some of these functions if users want to use elsewhere in their code I guess, but they should all be pretty simple.

Another note is that this is related to implementing Transforms. We could just not have merge strategies built-in and leave them up to the user to implement as transforms. If we do have merge strategies built-in, then the order between them and transforms needs to be defined (and merge strategies should probably just be implemented as a transform).
I would assume the merge goes first, then future transforms, but I'm actually not sure that's optimal, or that having the merge order be pre-defined is intuitive. It may be more intuitive to have merge strategies be mutually exclusive with transforms, so the user has to explicitly add it to their list of transforms.
The only issue there is that redux-persist has transforms per key while state reconcilers are for the whole store. It might just make sense to have transforms in mst-persist be on the whole store as well -- the user chooses what to do with each key on their own, and this allows for transforms involving multiple keys as well. Migrations seem to be done that way as well, so that way a migration would just be a transform as well.
(Getting a bit too into the details of transforms in the wrong issue, but so I don't forget again, transforms should probably receive a dereferenced snapshot instead of an MST object so that mutable changes don't accidentally change the state of the running app. EDIT: and then I forgot again that we're acting on a snapshot by definition: all transforms run inside of onSnapshot)

@airtonix
Copy link

airtonix commented Aug 8, 2020

Since I don't really understand most of what you're talking about, but can mostly assume it's to do with how to persist selected parts of our tree:

Assume you're making a game, where you have something roughly like

Game: <Game>
  Player: <Player>
    Characters: <Character[]>
      - Id: <Guid>
         name: <string>
         type: <Ref<CharacterClass>>
         hp: <number>
         entities: <Entity[]>
         modifiers: <Modifier[]>
    currentCharacter: <Ref<Character>>
  Meta: <GameMeta>
    CharacterClasses: <CharacterClass[]>
    
  Zones: <Zone[]>
    - Id: <Guid>
      Entities: <Entity[]>
        - Id: <Guid>
           name: 'Loot Chest'
           entities: <Entity[]>
             -  Id: <Guid>
                name: 'Coins'
                qty: 100
      Layers: <MapLayer[]>
        - Id: <Guid>
           depth: <number>
      Map: <Map>
        tileMap: <String>
        tileSet: <String>
  currentZone: <Ref<Zone>>

Some of this I want to persist, some of it i want to come from initial data:

Game.Player > persist
Game.currentZone > persist
Game.Zones[] > initial data
Game.Zones[].Entities > persist

Suggestions on how to do this without:

  • merge strategy
  • rewriting the Store to delineate peristable vs non persistable items?

@agilgur5
Copy link
Owner Author

@airtonix sorry for the delayed response

but can mostly assume it's to do with how to persist selected parts of our tree:

Sorry that's not correct. This issue is about how to merge persisted data with data created during .create. It currently overwrites data from .create as that is applySnapshot's default. It does use the defaults defined inside of your model itself though.

Assume you're making a game, where you have something roughly like

This is a fairly complex model -- minimal examples are much more helpful and significantly easier to respond to. I saw this when you commented, but given the complexity here, I did not really have the time to respond. I also did not see your edits until now

Some of this I want to persist, some of it i want to come from initial data:

You might be looking for #27, but I can see that this would be useful for the initial data portion as well.

Suggestions on how to do this without:

  • merge strategy
  • rewriting the Store to delineate peristable vs non persistable items? [sic]
  1. I'd say the main workaround would be to delete persisted data and add the initial data after persist runs.
  2. The other would be to add some defaults to the model itself since applySnapshot will use those in the case of undefined (i.e. if no data is persisted for a part of a model). But due to the nested nature of your data, I don't think this would solve your problem without Add Deep Whitelists & Blacklists  #27. For simpler models, this does work though and is how I've done defaults myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request solution: workaround available There is a workaround available for this
Projects
None yet
Development

No branches or pull requests

2 participants