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

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

Open
2 tasks done
feclist opened this issue Apr 17, 2020 · 10 comments
Open
2 tasks done

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

feclist opened this issue Apr 17, 2020 · 10 comments
Labels
docs or examples Documentation or examples related

Comments

@feclist
Copy link

feclist commented Apr 17, 2020

Bug report

  • I've checked documentation and searched for existing issues
  • I've made sure my project is based on the latest MST version

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
Copy link

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
Copy link
Author

feclist commented Apr 19, 2020

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
Copy link

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
Copy link
Collaborator

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.

@coolsoftwaretyler coolsoftwaretyler added the docs or examples Documentation or examples related label Jun 30, 2023
@samtgarson
Copy link

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
Copy link

zcmgyu commented Mar 18, 2024

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
Copy link

samtgarson commented Mar 19, 2024

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
Copy link

zcmgyu commented Mar 19, 2024

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

@jamonholmgren
Copy link
Collaborator

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

@coolsoftwaretyler
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs or examples Documentation or examples related
Projects
None yet
Development

No branches or pull requests

6 participants