mobxjs / mobx-state-tree

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

safeReference throws when set to point at non-existent entity #1509

Open feclist opened 4 years ago

feclist commented 4 years ago

Bug report

Sandbox link or minimal reproduction code

https://codesandbox.io/s/mobx-state-tree-todolist-464dw?file=/index.js

Describe the expected behavior

When a safeReference gets set to a value that points to no entity it should go undefined. I would expect the reference to be safe in this case as well. Correct me if that is not what is supposed to happen. Would I need to sanity check every reference assignment I do to ensure the pointed at entity is actually present in the tree? Was hoping safeReference would do that for me.

Describe the observed behavior

Once a safeReference gets a pointer to an id of an entity that doesn't exist MST throws an error.

Thanks for going through this one!

Romanior commented 4 years ago

If you are looking for more architectural ideologic answer I think we should wait for the creator's answer, but I think it has something to do with philosophy behind reconciliation.

On more practical side to move your code/project forward I suggest to use { isValidReference} from 'mobx-state-tree' to check if the reference is valid.

feclist commented 4 years ago

Ah yes, I actually tried to use that helper method. Somehow I didn't seem to get it to work properly (docs were not entirely clear to me either).

I did find my way around this restriction for now, leaving this open for now as I am very curious for a more architectural insight!

Thanks for your response @Romanior!

samtgarson commented 1 year ago

I would also love to get some clarification on this—I assumed that safeReference would work in this case.

Our use case is that we have a network of resources with relationships between each other, which are sometimes fetched without the related resource.

When landing on a page which access a model's related resource, we trigger the network request to fetch the relation but if the model happens to be in the store already from a previous page, we get a "failed to resolve reference".

We'd like to be able to safely access the relationship, but have it return an empty array until the network request has completed and the reference can resolve to the newly saved resources, and ideally without having to manually use tryReference or isValidReference all over our app.

Would be useful to know if we're approaching this the wrong way, it seems like it could be a fairly common set up?

coolsoftwaretyler commented 1 year ago

Hey folks - sorry it's been so long without an answer here. I think that the original issue is a misunderstanding of how safeReference is intended to work.

In the API docs, there's a note that says:

Strictly speaking it is a types.maybe(types.reference(X)) (when acceptsUndefined is set to true, the default) and types.reference(X) (when acceptsUndefined is set to false), both of them with a customized onInvalidated option.

So I think the idea is that you can provide or start with undefined in your safeReference, but if you try to access an invalid reference you will still trigger the documented exception, which is what's going on in this original example.

I think the docs around this could be clarified, so I'm going to tag it as such.

I know it's been a while since anyone responded here, but if any of y'all disagree with my interpretation or want to discuss further, I'd be happy to chat! If this isn't relevant to y'all anymore, no worries and have a great weekend.

samtgarson commented 1 year ago

We wrestled with this for a bit and ended up writing a custom resolver (using get/set) which allows us to set whether a reference is "required" or not (and therefore whether it should raise on an unresolved reference).

I think docs would be a great place to start for this—even if it is technically all listed there I think it could be restructured to make it clearer the intention of this area of functionality, specifically that even with safeReference accessing an invalid reference will raise, and there's no way of avoiding that without custom resolution logic.

Thanks for getting back 🙌

zcmgyu commented 5 months ago

Hello @samtgarson, I'm experiencing a similar issue. In my app logic, references aren't always required to exist. For instance, in the TopicModel, the createdBy property refers to UserModel. However, when listing topics, fetching user details is unnecessary. I only need to retrieve user details when fetching the topic details. Could you kindly share your customized get/set function for handling references?

samtgarson commented 5 months ago

Hey @zcmgyu, for sure. It ended up being quite complex but it worked well for us. I've added the relevant code and specs here https://gist.github.com/samtgarson/f1a84096aa53786e9281bd36d4e2dc0a

Worth noting that due to legacy reasons our system did not have globally unique IDs, only unique within a resource. This required us to deserialize our reference properties into a specific format from our API, so that the type and ID were encoded into it, e.g. posts|1. It felt like a fairly tightly coupled set of functionality but once we had it set up it worked transparently. This wouldn't be necessary if you used globally unique IDs (e.g. UUIDs) which could be resolved globally in the state tree without needing the type.

zcmgyu commented 5 months ago

Appreciation to @samtgarson for the prompt reply. I'll be utilizing this gist as a point of reference for my project.

jamonholmgren commented 5 months ago

I think we need to solve this problem at a library level.

coolsoftwaretyler commented 5 months ago

Two approaches come to mind that could fix this without breaking the existing behavior:

  1. Add a new type to our reference system that doesn't throw this exception
  2. Modify this exception so it does not throw, if provided some configuration on instantiation

We could also change safeReference to behave as people often expect it will, but I'm a little wary of that approach. Would prefer to expand the API a little on this one to preserve backwards compatibility.