mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.91k stars 641 forks source link

Make identifiers mutable #887

Open mweststrate opened 6 years ago

mweststrate commented 6 years ago

Mutating identifiers should be an exception, but potentially it would be nice if it was possible to make sure that we can swap client side identifiers for server side generated ones. Should

broncha commented 5 years ago

Thinking about it a bit... This would open up a possibility to support offline capabilities. A model can be assigned a transient ID while its being synced to the backend. It could very well be added to the store, the user would have an instant feedback, and still show some indication that the model has not synced yet. And when it has an ID from the backend it would update the references and clean up. I could look into this more. Thanks for mobx and mst :)

broncha commented 5 years ago

@mweststrate did you have a try on this? Heres what I have tried

identifier.ts

    reconcile(current: INode, newValue: string) {
        if (current.storedValue !== newValue && current.parent) {
            current.parent!.updateIdentifier(newValue)
        }
        return super.reconcile(current, newValue)
    }

object-node.ts

    updateIdentifier(newIdentifier: string) {
        this.identifier = newIdentifier
        this.root.identifierCache!.changeIdentifier(newIdentifier, this)
    }

identifier-cache.ts

    changeIdentifier(newIdentifier: string, node: ObjectNode) {
        const identifier = node.identifier!

        if (!this.cache.has(newIdentifier)) {
            this.cache.set(newIdentifier, observable.array<ObjectNode>([], mobxShallow))
        }

        const set = this.cache.get(newIdentifier)!
        if (set.indexOf(node) !== -1) fail(`Already registered`)
        set.push(node)

        const prevSet = this.cache.get(identifier)!
        if (prevSet.indexOf(node) == -1) fail(`Already removed`)
        prevSet.remove(node)
    }

Heres my test

test("references are updated when identifier change", () => {
    const values: number[] = []
    const Book = types.model({ id: types.identifier, price: types.number })
    const BookEntry = types.model({ book: types.reference(Book) }).views(self => ({
        get price() {
            return self.book.price * 2
        }
    }))
    const Store = types.model({
        books: types.array(Book),
        entries: types.optional(types.array(BookEntry), [])
    })

    const s = Store.create({ books: [{ id: "1", price: 3 }] })
    unprotect(s)

    s.books.push(Book.create({ id: "3", price: 2 }))

    s.entries.push(BookEntry.create({ book: "3" }))
    expect(s.entries[0].book.price).toBe(2)

    s.books[0].id = "6"
    s.entries.push(BookEntry.create({ book: "6" }))
    expect(s.entries[1].book.price).toBe(2)
})

It throws an exception at s.entries.push(BookEntry.create({ book: "6" })) because the cache update operation has not completed yet. It seems direct assign to identifiers need to prevented.

terrysahaidak commented 5 years ago

Any update on this or workaround? I have an optimistic update in my application and wonder if I could just reassign a new ID but not removing and adding a new item from the server.

Davidhidalgo commented 5 years ago

This is absolutely necessary for offline capabilities. I'll follow this thread closely and I'll try to come up with ideas. The approach of @broncha seems interesting.

fullofcaffeine commented 4 years ago

Any workarounds? I'd like to add offline capabilities to a MST app of mine but I'm a bit lost.

Grolaf commented 3 months ago

Hey ! Some time elapsed since the last update in this post. Do someone find out a possible workaround ?

Maybe for further explanations if it was needed, this feature would be really interesting, as the creator of the post said, for server-side generated IDs.

If implemented, this feature will allow models to define a "pushToBackend" action (callable for example with myModelInstance.pushToBackend()) , which could update the instance by itself, preserving the developer from managing the API return himself/herself. This will save a drastic amount of time and technical complexity...

Is this breaking the philosophy of MST ?

Thanks for those who will answer !

coolsoftwaretyler commented 3 months ago

Hi @Grolaf - I don't know of anyone who is working on this particular feature. I haven't thought much about it, myself. If it's something you're very interested in, I'd be happy to see if we can make a plan and maybe put together a PR.

I don't know if this breaks any MST philosophy, but I also don't know yet if it doesn't, either, haha.

All that to say, I don't have a strong opinion one way or another, I don't think much work is happening on this. Would be open to starting work on it, but I'd want to learn more about the use cases and go about it quite carefully.

Sorry it's not a more interesting answer!

Grolaf commented 3 months ago

Thank you for answering @coolsoftwaretyler ! I would be happy to do it, but since I don't know about MST code yet, it will take me some time to dive in. I'll do it on my free time and probably try some things. Anyway, I think that I have time as this post is open since 2018 :smile: .

coolsoftwaretyler commented 3 months ago

Sounds good! As you make progress will you please respond here so we can coordinate? Thanks!