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] Rethinking references API #1282

Open
xaviergonz opened this issue May 10, 2019 · 11 comments
Open

[RFC] Rethinking references API #1282

xaviergonz opened this issue May 10, 2019 · 11 comments
Labels
brainstorming/wild idea Brainstorming / Wild idea enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate

Comments

@xaviergonz
Copy link
Contributor

xaviergonz commented May 10, 2019

Right now refs are kind of hard to grasp for users, since they are actually ids that hide a resolution logic to objects.

Also they exhibit the following problems:

  • they are hard for TS (because of their id / object duality), causing some ugly casts to as any when an id has to be assigned
  • there's no way to fix a reference once broken (because accessing the reference itself will throw, which is required for assignation)
  • checking if a reference is broken is kind of hard (tryResolveReference and friends were added for this, but still...)

The proposal is a new API for using refs, something like this:

const M = types.model({ ref: types.reference(whateverType) })

const m = M.create({ ref: "123" })

m.ref.valid // true if the ref is not broken, false if it is broken

m.ref.deref() // gets the object pointed at, throws if the ref is not valid
m.ref.safeDeref() // (not sure about this one, since you could just check ref.valid before) gets the object pointed at, undefined if not valid
m.ref.id // gets the id the ref is pointing to

m.ref.set(obj) // sets the ref to the id of the given object, throws if the obj has no id
m.ref.id = x // sets the ref to the id directly

Technically it could be a non breaking change by adding a new ref type (or actually two with safeReference), but that'd mean we'd end up with 4 possible ref types

Maybe something like types.ref / types.safeRef

Or maybe something best left for a major version?

@xaviergonz xaviergonz added brainstorming/wild idea Brainstorming / Wild idea require('@mweststrate') @mweststrate input is required! labels May 10, 2019
@xaviergonz xaviergonz changed the title [RFC] Rethinking references [RFC] Rethinking references API May 10, 2019
@mweststrate
Copy link
Member

Initial reaction: like!
Although it is kinda set it does leak the normalization abstraction.

Just a random idea, didn't think it through, what if we would keep things as is, but that asRef(m.ref) would return the object you described above? In that case people can always opt in to the more expicit api, but there is still only one reference type. (asRef(m.ref) would require a weird temporarily global flag, like with auto value unboxing, but I think it does read better than asRef(m, "ref")?

@xaviergonz
Copy link
Contributor Author

xaviergonz commented May 16, 2019

What about asRef(() => m.ref)
if we deref an invalid ref directly (when passing it as a direct arg) then it would throw before getting to the asRef function

The one thing I don't quite like though is that the typing would be still kind of hard to do...
Well, now that I think about it, it is not supported at all now, now you need to do m.ref = 5 as any, whereas with this would be done with
asRef(() => m.ref).id = 5

So something like this then?

asRef(() => m.ref): {
  readonly valid: boolean, // true if the ref is not broken, false if it is broken
  id: number | string // get or set id

  // not sure if the following ones would be needed, but I guess they would be cool if you want to pass the ref object around
  deref(): T, 
  safeDeref(): T | undefined, 
  setTarget(T)

  // alternatively
  target: T, // deref + setTarget
  readonly safeTarget: T | undefined // alternative to safeDeref()
}

The other thing I don't like is that you need to make sure to use the parent object when accessing the child or else it won't work, whereas asRef(m, "ref") wouldn't require an arrow function and would make this clearer, so it would be harder to mess the call up (and can be typed ok in TS), so I think that might be a better option actually.

@Amareis
Copy link

Amareis commented Jun 20, 2019

Hey, what about external (not-from-tree) refs?

@nspaeth
Copy link

nspaeth commented Jul 17, 2019

If this fixes #1284, it would dramatically simplify some complex models I have.

Are we currently awaiting a decision, or for someone to volunteer to implement?

@mweststrate
Copy link
Member

I think it would be great if @xaviergonz suggestion was implemented:

asRef(owner, propertyName) => {
  readonly valid: boolean, // true if the ref is not broken, false if it is broken
  readonly id: number | string // get

  deref(): T, 
  safeDeref(): T | undefined, 
  setTarget(T)
  setId(number | string)
}

@mweststrate
Copy link
Member

@Amareis those can already be implemented using custom refs.

@mweststrate mweststrate added enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate and removed require('@mweststrate') @mweststrate input is required! labels Jul 19, 2019
@k-g-a
Copy link
Member

k-g-a commented Jul 25, 2019

While impelmenting stuff described in #1355 I thought of adding special types referenceArray/referenceMap.

Just a basic API overview:

const myArrayType = types.array(SomeSubType); // it would be impossible to inline this into model
const modelType = types.model({
 items: myArrayType
 itemReferences: types.arrayReference(myArrayType , mode: 'standalone' | 'safe' | 'sync', custom?: ReferenceOptionsGetSet)
})

The mode parameter will enforce one of the following behaviours:

  1. 'standalone' is basically the same as type.array(type.reference(SubType)) - no custom add/delete handling
  2. 'safe' is basically the same as type.array(type.safeReference(SubType)) - safely handle delete from source, do not handle add
  3. 'sync' is as 'safe', but with add operation support

None of the variants assume 'two-way-sync' (i.e. adding to itemReferences does not modify the original, it just breaks if there is no match in items), but this could be simply implemented via custom.set.

The main benefit is that we could remove fair amount of logic brought by safeReference (internal hooks, tracking id nodes and so on).

New implementation birdview:

  1. we introduce two internal storages (let's name it sources and referrers for now) - one per tree, same as with IdentifiersCache
  2. array/map types get additional methods: registerReference/unregisterReference
  3. every array/map type registeres itself within corresponding sources storage upon tree creation and checks if there are any referrers
  4. every referenceArray/referenceMap type registers itself within corresponding referrers storage upon creation and checks if there are any sources
  5. when steps 3 or 4 hit the match, corresponding source's registerReference is called with referrer;
  6. didChange (or maybe even willChange?) handlers of array/map type begin to respect registered referrer's and alter them
  7. upon referrer destruction we unregister it from source
  8. upon source destruction we clear all referrer's

Steps 3 and 4 are needed, because any part of the tree could be 'hidden' inside optional and will appear later on, durning interaction with the tree.

To support custom references across trees types.arrayReference/types.mapReference could accept instance getter delegate:

const myArrayType = types.array(SomeSubType); // it would be impossible to inline this into model
const sourceModelType = types.model({ items: myArrayType })
const sourceModel = modelType.create(...)

const referrerModelType = types.model({
  refs: types.arrayReference(() => sourceModel.items, 'sync')
})

const referrerModel = referrerModelType.create();
referrerModel.refs // is fully syncronized with sourceModel.items

@k-g-a
Copy link
Member

k-g-a commented Jul 25, 2019

Even simpler API: types.arrayReference((self) => self.items, 'sync') - by having self we can always reference own tree's parts, but are free to get anything we want (as the last example in my previous comment), even types.arrayReference((self) => getEnv(self).someOtherInjectedStore.items, 'sync') for those who prefer DI over direct imports

@mweststrate
Copy link
Member

I really love the idea @k-g-a, especially having designated types for collections of references could greatly simplify stuff.

I think synced deletes should be the default (and non configurable ?)

Overal I think we might have designed references too transparent, I think in future improvements we should maintain explicit reference objects with an explicit api, so that we can introduce all the utility methods discussed above and also introduce lazy resolving / loading of references.

@Bnaya
Copy link
Member

Bnaya commented Aug 22, 2019

What prevents us to implement references as an external library?
All the things around creating and accessing the global registry of models of a type?
Maybe MST can expose these lower level apis
And let developers have their own flavour, and give the maintainers more freedom with breaking changes.

@terrysahaidak
Copy link

terrysahaidak commented Aug 5, 2020

I'm currently working on a similar implementation to the proposal by @xaviergonz.

It's based on a real model. Why do I need a real model? Because then I can use getParent on that and get the actual parent of the reference.

For example, here is the case.
We have PostModel which has a reference to ApplicationModel. That application is going to be stored in some Entities model since many models can reference it.

const ApplicationModel = types.model("Application", {
  id: types.identifierNumber
});

const PostModel = types.model("Post", {
  id: types.identifierNumber,
  application: types.reference(ApplicationModel)
});

const EntitiesModel = types.model("EntitiesModel", {
  applications: types.map(ApplicationModel),
  posts: types.map(PostModel)
});

const RootModel = types.model("Root", {
  posts: types.array(types.reference(PostModel)),
  entities: types.optional(EntitiesModel, {})
});

So now we can create our root:

const root = RootModel.create({
  posts: [1],
  entities: {
    posts: {
      1: {
        id: 1,
        application: 1
      }
    },
    applications: {
      1: {
        id: 1
      }
    }
  }
});

The whole thing works great with the current reference implementation. But there is one thing which gets tricky.

Since Application can basically be a part of the post, we need some access to it from the application, too.

But when we do getParent(self) in the Application model, we get Map as a parent because the map is a parent of the model, but the post is a parent of the reference to that Application model.

The only implementation I could think about - to use a real model and custom registry for all the references to some type.

// first of all we need some place to store all our references
const RefsRegistry = new Map<IAnyComplexType, Map<string, IStateTreeNode>>();

export function createEntityRef<T extends IAnyComplexType>(
  entityName: string,
  Model: T
) {
  // find the registry based on the model type
  let currentRegistry = RefsRegistry.get(Model);

  if (!currentRegistry) {
    currentRegistry = new Map();
    RefsRegistry.set(Model, currentRegistry);
  }

  const refModel = types
    .model(`${Model.name}Ref`, {
      // or can be a number
      id: types.number
    })

    .volatile(self => ({
      // we need internal ref to store our reference
      _refId: `${Model.name}Ref-${uuid()}`
    }))

    .views((self: any) => ({
      get current() {
        const entities = getRoot<any>(self).entities;

        const entityModel = entities[entityName];
        // currently I resolve it from the entityModel but it can be stored anywhere
        return entityModel.get(self.id) as Instance<T>;
      }
    }))

    .actions(self => ({
      // we store our reference as soon as this model created
      afterCreate() {
        currentRegistry!.set(self._refId, self);
      }
    }));

  return refModel;
}

With that fabric now we can replace our types.reference(ApplicationModel) with our custom ref model:

const applicationRef = createEntityRef('applications', ApplicationModel);

const PostModel = types.model("Post", {
  id: types.identifierNumber,
  application: applicationRef
});

In order to make it work out of the box, we need to add snapshotProcessor to the ref model so it can receive number or string as a snapshot and convert it to { id: snapshot }.

Now we can use this function inside of ApplicationModel to get the parent of type Post of the ref (or parents):

export function getReferenceParentOfType<T extends IAnyModelType>(
  model: IStateTreeNode,
  parentType: T
): T["Type"] | undefined {
  const type = getType(model);
  const registry = RefsRegistry.get(type);

  if (!registry) {
    return undefined;
  }

  for (let value of registry.values()) {
    try {
      return getParentOfType<T>(value, parentType);
    } catch {}
  }

  return undefined;
}

And still, this works great. But there is another problem - mobx-state-tree is lazy by default. That means this block will be executed only when I actually access this ref.

      afterCreate() {
        currentRegistry!.set(self._refId, self);
      },

This is probably OK for most of the cases, but there is another cool one.

The other really useful case is to be able to find all the references to the specific model and be able to work with those references (for example, to remove from all the lists):

export function resolveModelReferences<T extends IAnyModelType>(
  type: T
): undefined | any[] {
  const registry = RefsRegistry.get(type);

  if (!registry) {
    return undefined;
  }

  return [...registry.values()];
}

But this will resolve all the accessed reference models because rest of them won't be stored inside the registry of the specific model type so again this block won't be executed until I access the reference model:

      afterCreate() {
        currentRegistry!.set(self._refId, self);
      },

Any ideas on how to work around this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorming/wild idea Brainstorming / Wild idea enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate
Projects
None yet
Development

No branches or pull requests

7 participants