feat: Add persisted state migrations

[damassi]

Similar to redux-persist, this PR adds the ability to migrate persisted state using a new migrations option on the persist helper. It looks like this:

const memoryStorage = createMemoryStorage({
  '[EasyPeasyStore][0]': {
    foo: "foo-updated",
    migrationConfict: "error"
  },
});

const store = createStore(
  persist(
    {
      foo: {
        bar: "bar",
      },
      bar: "bar",
    },
    {
      storage: memoryStorage,
      migrations: {
        migrationVersion: 1,

        0: (state) => {
          state.foo = { bar: state.foo };
        },
        1: (state) => {
          delete state.migrationConfict;
        },
      },
    }
  )
);

Yielding this updated store representation:

{ 
  "foo": { 
    "bar": "foo-updated" 
  }, 
  "bar": "bar", 
  "_migrationVersion": 1 
}

How it works:

• Numeric object keys are added to the migrations object
• Each migration function is an immer draft updator, which takes state
• The migrator will look at the currently stored internal version (state._migrationVersion) and perform migrations up to migrations.migrationVersion, as defined in the store configuration setup.

Questions:

1. We need to persist the currently migrated version, as that value is what's compared against for future migrations. Right now it's been added as _migrationVersion but perhaps there's a better place for this?
2. Anything I might be missing in the implementation? It felt surprisingly straightforward.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
easy-peasy	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 18, 2023 1:19am

[damassi]

Didn't want to use version here so as to not confuse the top-level persistKey version. migrationVersion seems sufficiently unambiguous

[no-stack-dub-sack]

Thanks for the PR! This is a cool concept which solves for what seems like a pretty common use-case. We will take a closer look at your changes / questions, and since this introduces a completely new feature, and since he's the most familiar with the persist implementation, I think we'll need @ctrlplusb to take a look and give his final sign off. If accepted, it would be nice to get this in before the upcoming 6.0.0 release which fixes a few minor issues and stabilizes the unstable_effectOn api.

[damassi]

Amazing. We've been using our own version of this on top of easy-peasy for a few years in our mobile app and its been working great but felt it would be better to simplify things by PRing upstream. Thanks for giving it a look and happy to finish up all of the docs and so on should y'all want to proceed 👍

[damassi]

Hi @ctrlplusb - any chance you would have a moment to give this PR a review?

[no-stack-dub-sack]

@damassi it looks like we're moving towards accepting this feature, likely to include as part of the upcoming v6 release which stabilizes unstable__effectOn. Apologies for the delay on this.

Before accepting this, though, we'd want to round it out with some docs and perhaps some more exhaustive tests. Is this something you'd be up for taking on?

As far as _migrationVersion, I think this is OK for now, but I'll take another look as well and see if there's maybe a better place for this.

Thanks!

[damassi]

I can certainly add docs! And more tests cases. I'll try to wrap this up today / tomorrow.

[no-stack-dub-sack]

This is looking solid @damassi! The docs look great (the example is very clear and helpful) and the tests are all passing. One small typo that's not even yours, so that's optional. Let's let @jmyrland have a final weigh in here as well but this is looking good from my end.

Nice work!

[no-stack-dub-sack]

@damassi @jmyrland On second thought about the docs, what do you think about moving the section about the default behavior up to right under the "Managing Model Updates" header, then introducing the two options, so it could read something like:

---

Managing model updates

It's entirely reasonable that your store model will evolve over time. However, there is an inherent risk with this when utilizing the persist API.

If your application has previously been deployed to production then users may have persisted state based on a previous version of your store model. The user's persisted state may not align with that of your new store model, which could result in errors when a component tries to consume/update the store.

By default the persist API utilizes the mergeDeep strategy (you can read more above merge strategies further below). The mergeDeep strategy attempts to perform an optimistic merge of the persisted state against the store model. Where it finds that the persisted state is missing keys that are present in the store model, it will ensure to use the respective state from the store model. It will also verify the types of data at each key. If there is a misalignment (ignoring null or undefined) then it will opt for using the data from the store model instead as this generally indicates that the respective state has been refactored.

Whilst the mergeDeep strategy is fairly robust and should be able to cater for a wide variety of model updates, it can't provide a 100% guarantee that a valid state structure will be resolved.

Therefore, when dealing with production applications, we recommend that you consider removing this risk by using one of the two options described below:

Migrations

<your docs here>

Forced updates via version

If migrations are insufficient (which can often be the case after a major state refactor has taken place), the persist API also provides a means to "force update" the store via version. You can do so by utilizing the version configuration property that is available on the store config.

<original docs here>

[damassi]

Yup, I agree that this reordering works much better. Updated 👍

[jmyrland]

Some minor highlights missing, other than that - LGTM 👍

[no-stack-dub-sack]

@damassi @jmyrland I started thinking a bit more about how migrations and merge strategies may/may not work together and came up with this scenario. Let's say you have the following persisted state:

{
  city: 'london',
  postCode: 'e3 1pq',
  id: 1,
}

and you add a migration to update the model to the following type:

type State = {
  address: {
    city: string;
    postCode: string;
  };
  id: string;
}

However the migration you wrote only does this:

{
  1:  (state) => {
    state.address = {
      city: state.city,
      postCode: state.postCode,
    }

    delete state.city;
    delete state.postCode;
  }
}

But neglects to add state.id = String(state.id).

In this case, the structural change will be applied, but there will be a data type mismatch between id in the user's persisted state and id from the model, and since merge strategies are applied after migrations, the value from the store model will be used due to the mismatch.

I was originally thinking that merge strategies being applied after migrations would pretty much be a no-op, but I believe this example illustrates a use-case where both migrations and merge strategies will impact the resolved state (noting, however, that a better-written migration would have made the merge strategy a no-op).

I don't necessarily see this as being an issue, but do you think its worth calling out that merge strategies may work in conjunction with migrations, and that they're not always mutually exclusive?

[damassi]

From experience using this migration approach over the past few years, there is a good amount of implied responsibility put on the dev writing the migration due to the sensitivity of the task. For example, in our app we have another layer of migration-oriented tests to ensure correctness, where something like this couldn't happen.

As long as users writing migrations appreciate the risk (which is well outlined in the current docs, I think) it will prompt them to take the migration writing process seriously. If this can be emphasized further, however, all for it.

[jmyrland]

As long as users writing migrations appreciate the risk (which is well outlined in the current docs, I think) it will prompt them to take the migration writing process seriously.

Makes sense - it's in the hands of the developer at that stage.

If this can be emphasized further, however, all for it.

A short warning at the end of the migrations section might be warranted?

⚠ Whenever properties in your store changes types, ensure that you migrate these properly. 
Persisted data might be lost in when [merging](#merge-strategies), if there is a mismatch 
between types in the store & the persisted data (excluding `null` and `undefined`).

Maybe another short paragraph, to encourage writing tests for migrations to ensure correctness?

These additions might be overkill, and lead to more confusion/bloated docs. I'm guessing that people in need of migrations will test this properly before "release".

Thoughts @no-stack-dub-sack @damassi ?

[damassi]

I would vote for skipping since the persist docs are already quite long and descriptive. If one is at the point in their apps life-cycle where they're writing migrations then these issues will naturally be apparent (imo). More than happy to add whatever y'all think is needed though.

[no-stack-dub-sack]

@damassi I think I agree. I would either vote for skipping it or a simpler two-liner. Something like:

Well-written migrations should obviate the need for merge strategies and render them no-ops. Note, however, that merge strategies are still applied and unexpected behavior may occur during rehydration as a result of non-exhaustive migrations.

If either of you think that this may still invite confusion, I'm down to skip it since as @damassi said, developers at this stage in their app development should generally be aware of these issues and its on them to handle them correctly.

[damassi]

I actually quite like your suggestion @no-stack-dub-sack; I think that puts it well in just the right amount of words. If you'd like to proceed with it, let me know where exactly to drop it into the doc.

[no-stack-dub-sack]

@damassi I think it can go right before the example, as a new paragraph, or right after. It might make more sense after, but its possible it could also be missed there. I'd be ok with either place, though, so whatever you think makes the most sense.

[damassi]

Moved it below, where it reads / flows a bit better.

[no-stack-dub-sack]

Great work here @damassi! This is looking good. I've added one more test and I'm merging this in. @jmyrland and I will work on prepping a v6 release very soon 👍

[no-stack-dub-sack]

LGTM!

[damassi]

Thanks all! Very excited to pull out all of our homegrown code and drop this into our apps, and simplify. We <3 easy peasy over at Artsy.