refactors, onInvalidation hook, types.safeReference and more #1091

xaviergonz · 2018-11-24T12:10:53Z

small refactor over references
small refactor over scalar and object nodes, by extracting common code to a BaseNode class
added an internal mechanism to subscribe to hooks (all of them for object nodes, afterAttach/beforeDestroy for scalar nodes)
- this should make it kind of trivial to make a addHook(model/array/map/scalar/whateverType, 'afterCreation'..., (self) => {...}) in the future if desired
added an onInvalidated option to the reference options (explained below)
added a types.safeReference type (explained below)
note that isValidReference and tryReference were alredy merged in tryReference and isValidReference #1087 this just adds extra docs for them

Reference validation: `isValidReference`, `tryReference`, `onInvalidate` hook and `types.safeReference`

Accessing an invalid reference (a reference to a dead/detached node) triggers an exception.

In order to check if a reference is valid, MST offers the isValidReference(() => ref): boolean function:

const isValid = isValidReference(() => store.myRef)

Also, if you are unsure if a reference is valid or not you can use the tryReference(() => ref): ref | undefined function:

// the result will be the passed ref if ok, or undefined if invalid
const maybeValidRef = tryReference(() => store.myRef)

The options parameter for references also accepts an optional onInvalidated hook, which will be called when the reference target node that the reference is pointing to is about to be detached/destroyed. It has the following signature:

const refWithOnInvalidated = types.reference(Todo, {
    onInvalidated(event: {
        // what is causing the target to become invalidated
        cause: "detach" | "destroy" | "invalidSnapshotIdentifier"
        // the target that is about to become invalidated (or undefined if the cause is "invalidSnapshotIdentifier")
        invalidTarget: STN | undefined
        // the identifier that is about to become invalidated
        invalidId: string | number
        // parent node of the reference (not the reference target)
        parent: IAnyStateTreeNode 
        // a function to remove the reference from its parent (or set to undefined in the case of models)
        removeRef: () => void 
        // a function to set our reference to a new target
        replaceRef: (newRef: STN | null | undefined) => void 
    }) {
        // do something
    }
})

Note that invalidation will only trigger while the reference is attached to a parent (be it a model, an array, a map, etc.).

A default implementation of such onInvalidated hook is provided by the types.safeReference type. It is like a standard reference, except that once the target node becomes invalidated it will:

If its parent is a model: Set its own property to undefined
If its parent is an array: Remove itself from the array
If its parent is a map: Remove itself from the map

Strictly speaking it is implemented as

types.maybe(types.reference(Type, {
    ...customGetSetIfAvailable,
    onInvalidated(ev) {
        ev.removeRef()
    }
}))

const Todo = types.model({ id: types.identifier })
const Store = types.model({
    todos: types.array(Todo),
    selectedTodo: types.safeReference(Todo)
})

// given selectedTodo points to a valid Todo and that Todo is later removed from the todos
// array, then selectedTodo will automatically become undefined

… nodes

xaviergonz · 2018-11-25T08:32:02Z

Renamed safeReference to weakReference, it seems like a better name

xaviergonz · 2018-11-25T17:45:07Z

all done

k-g-a · 2018-11-27T13:31:00Z

Do you think it can be that bad?

I think that one level won't be noticable at all, but I am not sure, need measurement.
Will try some profiling next week. But I think that this point should not block PR from being merged!

wouldn't that throw upon access? the refs and the source reside in a different "tree"

I've just discovered, that this code was perfectly valid only up to 3.7.0 (including, https://codesandbox.io/s/vmq29xm1yl) - 3.7.1 broke it. Ability to have multiple trees is one of MST's top features. Reusing nodes from one tree in another was a bit of a pain, but it worked. Could we bring that behaviour back?

Found a way to do it without actually instantiating references (at least for the case without the custom get/set)

Didn't get that point. If one's reference source resides in another tree he must supply instances on create, not identifiers/snapshots. Othervise IdentifierCache should have been global and shared between all root nodes.

const Source = types.model({items: types.array(MyType)})
const Refs = types.model({linkedItems: types.array(types.reference(MyType))})
const source1 = Source.create({items: [{id: 1}, {id: 2}, {id: 3}]})
const source2 = Source.create({items: [{id: 1}, {id: 4}, {id: 5}]})
// at this point we could not supply ids, because id=1 lives in both source1 and source2
const refs = Refs.create({linkedItems: source1.items})

xaviergonz · 2018-11-27T17:49:05Z

@k-g-a

I think that one level won't be noticable at all, but I am not sure, need measurement.
Will try some profiling next week. But I think that this point should not block PR from being merged!

Fair enough :)

I've just discovered, that this code was perfectly valid only up to 3.7.0 (including, https://codesandbox.io/s/vmq29xm1yl) - 3.7.1 broke it. Ability to have multiple trees is one of MST's top features. Reusing nodes from one tree in another was a bit of a pain, but it worked. Could we bring that behaviour back?

I didn't even know that was supposed to be possible. I guess that's something for @mweststrate to decide?

Didn't get that point. If one's reference source resides in another tree he must supply instances on create, not identifiers/snapshots. Othervise IdentifierCache should have been global and shared between all root nodes.

Well, my answer was assuming that references and referenced nodes had to share a common root. Of course if they don't there's no way check if references are valid at creation time.

Just wondering, if refs can be assigned to referenced nodes in other stores, what's an id assignation default behaviour? taking your example code, what should this do?

refs.linkedItems[0] = "5" // some id... from which store... ??

Also, if you restore a snapshot, how will the reference know into which store it should hook to find the node?

xaviergonz · 2018-11-27T17:57:58Z

Hmm the docs say https://github.com/mobxjs/mobx-state-tree#references

References are looked up through the entire tree but per type, so identifiers need to be unique in the entire tree.

k-g-a · 2018-11-28T20:09:05Z

taking your example code, what should this do?

Throw ))
I know that using reference source from other tree is a kind of a hack and is only possible in limited scenarios. I won't even argue if this will be forbidden: there is always an alternative way through volatile state from env :)
If we really want to allow such approach, we could make shared IdentifierCache: this would be breaking change. Or won't if we make it under option or as a separate type. Anyway, I think that it's for @mweststrate to decide.

mweststrate · 2018-11-30T10:46:44Z

Renamed safeReference to weakReference, it seems like a better name

Wouldn't this be confusing with the weakReference proposal in javascript? https://github.com/tc39/proposal-weakrefs/blob/master/specs/weakrefs.md

mweststrate · 2018-11-30T10:54:08Z

taking your example code, what should this do?

Throw ))
I know that using reference source from other tree is a kind of a hack and is only possible in limited scenarios. I won't even argue if this will be forbidden: there is always an alternative way through volatile state from env :)
If we really want to allow such approach, we could make shared IdentifierCache: this would be breaking change. Or won't if we make it under option or as a separate type. Anyway, I think that it's for @mweststrate to decide.

The idea I had was that standard types.reference doesn't work for different trees, however, that custom references would be able to resolve inside a different tree, by using resolveIdentifier and such:

types.reference(User, {
        // given an identifier, find the user
        get(identifier) {
            return resolveIdentifier(someOtherStore /* this is another tree, accessed from closure, import, env or whatever */ , User /* type (living in the other tree) */, identifier)
        },
        // given a user, produce the identifier that should be stored
        set(value /* User */) {
            return value.name
        }
    })

mweststrate · 2018-11-30T10:55:23Z

@xaviergonz didn't review the code yet, since not all conceptual questions are answered :-). But this PR looks like a great improvement!

xaviergonz · 2018-11-30T18:17:02Z

@mweststrate Thanks for the feedback :)

Wouldn't this be confusing with the weakReference proposal in javascript?

Didn't even know that existed. I'll switch it back to safeReference then

The idea I had was that standard types.reference doesn't work for different trees, however, that custom references would be able to resolve inside a different tree, by using resolveIdentifier and such:

Ah, so it is supposed to be ok for custom refs.
Ok, then I'll get rid of the "invalidSnapshotIdentifier" cause (since that needs snapshots of the reference targets on the same tree than references themselves) and add some tests for custom refs that point to other stores.

Thanks!

xaviergonz · 2018-11-30T19:14:36Z

Ok, I was able to keep the invalidSnapshotIdentifier even for custom identifiers in another stores

(basically in this case upon creation of the safe reference, it will try to access the node, thus trigger the getter, and if that getter does not return a node then the reference will trigger the invalidSnapshotIdentifier cause)

Also I wrote a unit test to make sure that custom identifiers can use nodes from external trees (it seems it keeps working)

xaviergonz · 2018-11-30T19:15:12Z

Also renamed it back to safeReference

xaviergonz · 2018-11-30T19:17:46Z

const Source = types.model({items: types.array(MyType)})
const Refs = types.model({linkedItems: types.array(types.reference(MyType))})
const source = Source.create({items: [{}, {}, {}]})
const refs = Refs.create({linkedItems: source.items})

@k-g-a added a unit test for that, but using custom references (it works)

mweststrate

Awesome work!

xaviergonz · 2018-12-03T18:28:02Z

Thanks! last thing, renamed oldRef to invalidTarget and added invalidId to the event

k-g-a · 2018-12-05T07:44:41Z

@xaviergonz, thanks for great work! Could you estimate how soon will it land? :)

xaviergonz · 2018-12-05T07:47:28Z

I'll make a new release this afternoon

xaviergonz · 2018-12-05T17:39:23Z

@k-g-a 3.9.0 released

k-g-a · 2018-12-07T10:49:17Z

@xaviergonz, refactored our production code from volatile state to safeReferences: it works great and reduces boilerplate. Thanks for contribution!

xaviergonz · 2018-12-07T10:58:01Z

@k-g-a my pleasure :D

xaviergonz added 6 commits November 21, 2018 20:28

implementation

a36d41e

renamed to tryReference

8d182d2

updated changelog

86e671b

check that reaction can be used to clear invalid references

86bb04b

refactor of nodes and references, added a hook subscription to scalar…

266a943

… nodes

finished the implementation

971d819

xaviergonz requested review from mweststrate and k-g-a November 24, 2018 12:11

xaviergonz added 5 commits November 24, 2018 13:12

updated changelog

dca90b3

use this rather than super

077ac88

merged latest master

110a0c3

added test for cloning

07371f0

rebuilt docs

99931d4

xaviergonz self-assigned this Nov 25, 2018

xaviergonz added the enhancement Possible enhancement label Nov 25, 2018

xaviergonz mentioned this pull request Nov 25, 2018

Reference lost from tree #1086

Closed

2 tasks

xaviergonz changed the title ~~reference, onInvalidation hook, types.safeReference and more~~ refactors, onInvalidation hook, types.safeReference and more Nov 25, 2018

renamed safeReference to weakReference

8e9c69e

xaviergonz changed the title ~~refactors, onInvalidation hook, types.safeReference and more~~ refactors, onInvalidation hook, types.weakReference and more Nov 25, 2018

xaviergonz added 9 commits November 25, 2018 09:38

fixed readme typo

434f67e

tweaked a bit the onInvalidated api

bbeb5ce

small readme fix

2e37ec9

readme fix

6cc43f4

fixed readme once more

efcbdc2

better construction error recovery

48c87a4

better constructor error recovery 2

93b80b3

removed unused utils methods

0203b08

set parent of scalar node to null on death

e0cf0e0

xaviergonz added the require('@mweststrate') @mweststrate input is required! label Nov 27, 2018

xaviergonz added 2 commits November 30, 2018 19:21

renamed weakReference back to safeReference

c0ecb97

making sure custom refs to another stores work

2d7583a

xaviergonz changed the title ~~refactors, onInvalidation hook, types.weakReference and more~~ refactors, onInvalidation hook, types.safeReference and more Nov 30, 2018

merged master

f715d62

k-g-a approved these changes Dec 3, 2018

View reviewed changes

mweststrate approved these changes Dec 3, 2018

View reviewed changes

xaviergonz added 2 commits December 3, 2018 19:24

renamed oldRef to invalidTarget, added invalidId

465e98a

fixed readme typo

d210675

added better argument types for emit in EventHandlers

076469c

xaviergonz merged commit a15d516 into master Dec 3, 2018

k-g-a mentioned this pull request Dec 25, 2018

Is it possible to assign by identifier? #1123

Closed

2 tasks

xaviergonz deleted the safeReference branch December 29, 2018 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactors, onInvalidation hook, types.safeReference and more #1091

refactors, onInvalidation hook, types.safeReference and more #1091

xaviergonz commented Nov 24, 2018 •

edited

Loading

xaviergonz commented Nov 25, 2018 •

edited

Loading

xaviergonz commented Nov 25, 2018

k-g-a commented Nov 27, 2018 •

edited

Loading

xaviergonz commented Nov 27, 2018 •

edited

Loading

xaviergonz commented Nov 27, 2018

k-g-a commented Nov 28, 2018 •

edited

Loading

mweststrate commented Nov 30, 2018

mweststrate commented Nov 30, 2018

mweststrate commented Nov 30, 2018

xaviergonz commented Nov 30, 2018 •

edited

Loading

xaviergonz commented Nov 30, 2018 •

edited

Loading

xaviergonz commented Nov 30, 2018

xaviergonz commented Nov 30, 2018 •

edited

Loading

mweststrate left a comment

xaviergonz commented Dec 3, 2018 •

edited

Loading

k-g-a commented Dec 5, 2018

xaviergonz commented Dec 5, 2018

xaviergonz commented Dec 5, 2018

k-g-a commented Dec 7, 2018

xaviergonz commented Dec 7, 2018

refactors, onInvalidation hook, types.safeReference and more #1091

refactors, onInvalidation hook, types.safeReference and more #1091

Conversation

xaviergonz commented Nov 24, 2018 • edited Loading

Reference validation: isValidReference, tryReference, onInvalidate hook and types.safeReference

xaviergonz commented Nov 25, 2018 • edited Loading

xaviergonz commented Nov 25, 2018

k-g-a commented Nov 27, 2018 • edited Loading

xaviergonz commented Nov 27, 2018 • edited Loading

xaviergonz commented Nov 27, 2018

k-g-a commented Nov 28, 2018 • edited Loading

mweststrate commented Nov 30, 2018

mweststrate commented Nov 30, 2018

mweststrate commented Nov 30, 2018

xaviergonz commented Nov 30, 2018 • edited Loading

xaviergonz commented Nov 30, 2018 • edited Loading

xaviergonz commented Nov 30, 2018

xaviergonz commented Nov 30, 2018 • edited Loading

mweststrate left a comment

Choose a reason for hiding this comment

xaviergonz commented Dec 3, 2018 • edited Loading

k-g-a commented Dec 5, 2018

xaviergonz commented Dec 5, 2018

xaviergonz commented Dec 5, 2018

k-g-a commented Dec 7, 2018

xaviergonz commented Dec 7, 2018

xaviergonz commented Nov 24, 2018 •

edited

Loading

Reference validation: `isValidReference`, `tryReference`, `onInvalidate` hook and `types.safeReference`

xaviergonz commented Nov 25, 2018 •

edited

Loading

k-g-a commented Nov 27, 2018 •

edited

Loading

xaviergonz commented Nov 27, 2018 •

edited

Loading

k-g-a commented Nov 28, 2018 •

edited

Loading

xaviergonz commented Nov 30, 2018 •

edited

Loading

xaviergonz commented Nov 30, 2018 •

edited

Loading

xaviergonz commented Nov 30, 2018 •

edited

Loading

xaviergonz commented Dec 3, 2018 •

edited

Loading