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

[RFC] Transforms & Migrations (& Plugins?) #23

Open
agilgur5 opened this issue Nov 29, 2019 · 2 comments
Open

[RFC] Transforms & Migrations (& Plugins?) #23

agilgur5 opened this issue Nov 29, 2019 · 2 comments
Labels
kind: feature New feature or request

Comments

@agilgur5
Copy link
Owner

agilgur5 commented Nov 29, 2019

Motivation

This RFC is meant to discuss what the future of transforms might look like now, so that we have less breaking changes in the future to a potentially heavily used API. The big aspects of that future, in my mind at least, are Migrations and possibly Plugins (see below). There are probably use-cases I'm not foreseeing as well.

Transforms

I think the proposal for transforms that I have out in #16 has reached a solid state and is pretty good API now. If anyone's got a minute to review it, that would be very welcome!

The main question I have around the Transform API proposed there is whether ITransformArgs should accept more than just a snapshot. Like, say, if the user needs to operate on the store itself, or needs an option specified in the config. For example, merge strategies could be implemented as a transform, but it would need the store in order to a shallow or deep merge against it:

import { getSnapshot } from 'mobx-state-tree'
import { persist, ITransform } from 'mst-persist'

import { UserStore } from './models'

const user = UserStore.create({name: 'Joe'})
await persist('user', user, {
  transforms: [shallowMerge]
})

const shallowMerge: ITransform = {fromStorage: (snapshot, store) => {
  // get the store's initial state from .create() and any actions done prior to calling persist
  const newSnapshot = { ...getSnapshot(store), ...snapshot }
  return newSnapshot
}}

That being said, this could easily just be implemented as a function/closure like all the other internal transforms, it's just a bit of an awkward API as you would pass the store both to persist and to the "transform creator" let's call it:

import { getSnapshot } from 'mobx-state-tree'
import { persist, ITransform } from 'mst-persist'

import { UserStore } from './models'

const user = UserStore.create({name: 'Joe'})
await persist('user', user, {
  transforms: [shallowMerge(user)]
})

function shallowMerge (store) {
  const transform: ITransform = {fromStorage: (snapshot) => {
    // get the store's initial state from .create() and any actions done prior to calling persist
    const newSnapshot = { ...getSnapshot(store), ...snapshot }
    return newSnapshot
  }}
  return transform
}

This way, the "transform creator" way, there's no need to pass any additional arguments to a transform internally, and it's a lot cleaner of an API, albeit more awkward. This is a more composable approach than adding more arguments. All of the options could similarly be passed to any transform -- but that would require duplicating them (and potentially several times if you have multiple complex transforms). "Transform creators" could also make for better typing, as you could be more specific about the types of the arguments passed to your closure.

Migrations

Migrations, if they are to be implemented in this library, would be built on top of the Transform functionality above.

I think redux-persist's Migrations API is pretty good and makes some good trade-offs. The main thing I would change, that is highly requested in redux-persist itself, is specifying an upgrade and downgrade path (similar to how an ORM does migrations as well). I would also move the version argument into the transform itself, something like:

interface IMigrate {
  (version: number, migrations: IMigration[]): ITransform
}

interface IMigration {
  version: number, // this should be unique
  upgrade?: ITransformArgs,
  downgrade?: ITransformArgs,
}

But really the big question here is, should we even implement Migrations in this library? As in, due to the myriad ways a user models their data, does it make more sense to let the user handle migrations themselves in their store or elsewhere? Or should we give them a "best practice" built-in, but let them swap it out if they need to?

I was initially thinking we shouldn't, for a few reasons:

  1. Where do we store the version #? We have some internal logic around storage that really shouldn't be imported if it is meant to be a transform.
    The easiest solution is probably to hijack the snapshot instance and add some mst-persist specific attributes, which I believe is what redux-persist does with its state._persist.version logic (but not sure as I don't totally understand all the redux-persist internals). This would be removed prior to applySnapshot ofc.
    We could also use this to store things like the current mst-persist version or the version of a transform, which could be useful in the future to warn/error against breaking changes or auto-detect and auto-migrate the breaking change (which might not be breaking anymore then).
    This might put some ordering constraints on the migrate transform however and may make things like changing storage engines more difficult (or maybe not, idk)
  2. It moves the version away from the schema. Ideally the version should be as close to the schema as possible. We can workaround this by requesting users to add a special property or map to their model, but that adds a barrier to adoption / usage.

After comparing it more to how ORMs handle migrations, I think it does make more sense to have a "best practice" built-in. ORMs do typically store version numbers in a DB table (a separate one from the model's table), as well as information as to which migrations have been applied or not. There are typically no version #s in the schema definition either.
And, in general, migrations are a fairly common need for a persistence library, so having some out-of-the-box way of handling them is likely better than none, even if the out-of-the-box way is not necessarily the best for everyone's use case. We may not have a "best practice" default initially, but we should ideally strive to have the "best practice" built-in. Making it a transform that's easily swappable allows for competition and innovation.

The ORM comparison gave me some thoughts around whether migrations should be an array or object, whether/how to store which migrations have been run or not, whether to use an auto-generated hash of the schema or not, etc. Might want to examine more ORM code.
To an extent, I think that may add more complexity than necessary, especially for an MVP, and just going with a single version that is an integer that only moves up or down by one is much easier to reason about. We could iterate based on feedback from there.
There are still some questions on error handling that arise even from that, e.g. if migrations contain versions 7, 8, 10, but not 9 (because 7 ate 9) or other such gap, do we error out?, if migrations are not integers, do we error out?, if the current persisted version of data is 5 and we only have migrations for 7+, do we error out? (this last one would likely error on applySnapshot anyway), etc

Plugins

The concept of plugins would basically be a more generic version of transforms that is any to any type and allows you to swap out basically all the internal functionality of mst-persist. Basically, JSON.parse/JSON.stringify, onSnapshot/applySnapshot, and storage.getItem/storage.setItem could be implemented as plugins. Which would mean one could change them and fully customize everything if wanted. They could also change up the ordering of everything if wanted (though, if you're swapping out everything, you don't really need this library at all then). Transforms vs. Plugins would be kind of similar to Webpack's Loaders vs. Plugins.

Some "plugins" could be implemented as say, noop transforms that just have side-effects, but others might need to actually have different return types. While this could be implemented elsewhere, a plugin might be a logical place for some "migrateStorage" functionality that lets you move from one storage engine to another.

Plugins are more future-oriented and not necessarily going to be implemented soon (or ever), but I think the concept is relevant to think about regarding "the future of transforms", especially as transforms are a subset of plugins.


Any and all feedback, comments, and discussion is welcome! Thanks for providing your input 🙂

@agilgur5 agilgur5 mentioned this issue Dec 4, 2019
5 tasks
@agilgur5
Copy link
Owner Author

agilgur5 commented Dec 4, 2019

@Venryx @barbalex @rafamel would love to get any of your input on this 🙂

@barbalex
Copy link
Contributor

barbalex commented Dec 4, 2019

@agilgur5

It is very comforting to see that this project (and you) has a vision. When I get such ideas in my own projects, that's when things are developing nicely and I enjoy working.

But to be honest: I am simply using mst-persist so that users can continue working where they left off the last time they visited my app. When reading your notes I wonder if I have missed some relevant points when changing data structure once mst-persist is implemented. If I encounter any problems I may invest more time to understand more in the future. But at the moment I am very happy with the way it works and I don't feel that I could be of any help.

Hope someone else can be more helpful.

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
Projects
None yet
Development

No branches or pull requests

2 participants