onInvalidated not Called when Bad Reference Hits Store - App Crashes - React Native Expo

[ChristopherGabba]

Bug report

• I've checked documentation and searched for existing issues and discussions
• I've made sure my project is based on the latest MST version
• Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

1. Clone this repo: https://github.com/ChristopherGabba/BugHuntingForFrankAndTyler.git
2. Create expo development build via simulator or via actual device: npx eas build --profile development --platform ios
3. Run npx expo install -c in terminal
4. The ignite boilerplate app will open up. Tap Sign in
5. Go to the Podcasts Tab at the bottom
6. Tap on any Toggle Favorite button for any podcast. I hard coded this:

export const EpisodeStoreModel = types
  .model("EpisodeStore")
  .props({
    episodes: types.array(EpisodeModel),
    favorites: types.array(
      types.safeReference(EpisodeModel, {
        onInvalidated(event) {
          console.log("INVALIDATED", JSON.stringify(event, null, 4))
        },
      }),
    ),
    favoritesOnly: false,
  })

with this:

    addFavorite(episode: Episode) {
      const bsEpisode: ReferenceIdentifier[] = ["BS_EPISODE_REFERENCE"]
      store.setProp("favorites", bsEpisode)
    },

Describe the expected behavior

I would expect the onInvalidated hook to call and remove the bad reference immediately instead of crashing the app.

I've SEVERELY struggled with this behavior with MST with regard to references, and that's why I migrated to safeReference so that it wouldn't crash. The app should not crash when a bad reference arrives, it should handle gracefully (aka hit the invalidated function call).

Real Life Scenario:

1. Fetch data from backend
2. Data was modified by another user (lets say a Todo action and assigned it to another user that isn't in the store)
3. Fetch data again
4. Bad reference arrives and app crashes

Describe the observed behavior

The app crashes immediately.

You'll notice after you add the bad reference, the app is stuck in an eternal cycle of instant crashes because the bad reference is just stuck in the store, and it forces the dev to have to clear the store. When you have about 10 reference situations in an app like mine, that is so annoying to have to do.

In my app, what I've had to do is:

    addFavorite(episode: Episode) {
      const bsEpisodes: ReferenceIdentifier[] = ["BS_EPISODE_REFERENCE"]
      // Install a function that parses episodes first and checks for valid references
      const validEpisodes = checkEpisodesForValidReferences(bsEpisodes)
      store.setProp("favorites", validEpisodes)
    },

And I do not enjoy having to do this, and frankly feel as though I shouldn't have to if the onInvalidated function was working correctly. If a reference is bad from the backend, you have to send something to modify the backend to make sure it doesn't just keep hitting the client with a bad reference over and over again, and the onInvalidated hook was where I handle this..

There has to be a better way of dealing with this.

As always, thanks so much for looking into this, I love MST (minus the reference stuff) :)

[ChristopherGabba]

I ran across a similar issue here: #1509

And I wholeheartedly agree with @jamonholmgren's comment!

[coolsoftwaretyler]

Hi @ChristopherGabba - thanks for the bug report and I'm sorry for all the frustration.

For what it's worth, I agree that this is a poorly documented part of the codebase, and it's harder than it has to be. I agree with Jamon that we should solve this at a library level.

I am happy to keep this open along with the other issue, and we'll continue to consider how to build a feature that does what you (and many other people) expect.

But I do want to bring up my comment from that thread: #1509 (comment)

Strictly speaking, the behavior is working as intended. I'm not certain if it will make sense to change safeReference behavior, or if we'll need to make a new type of reference.

I think the main problem here is the naming and documentation of this feature makes it seem like it's going to do something very different from what it does in actuality. A safeReference is just a reference that allows you to pass a valid identifier OR the literal value undefined. safeReferences are not something that allow you to pass an invalid identifier, although there is clearly a desire for behavior like that.

The onInvalidated hook isn't for handling invalid references (again, I think the naming is very confusing, haha). Rather, it is a hook for when a previously valid reference is about to be detached or destroyed: https://mobx-state-tree.js.org/concepts/references (search for "onInvalidated")

I don't say that to dismiss the concern. Just to clarify for anyone else who ends up here.

But still, I hear you (and many other folks), that this is a painful experience. I don't have a great estimate on when this will be fixed, or how.

If you're very motivated to have a better experience, I'd be very happy to help you get set up to contribute to the codebase, and I always prioritize community contributions for code review.

Could be a fun way to work together to resolve a longstanding pain point! If that's interesting to you, email me at tyler (at) coolsoftware.dev and I'd even be down to schedule a video call to onboard you.

If you don't have the time or interest, we'll just keep you updated here if and when we get around to figuring this out.

Have a great weekend!

[ChristopherGabba]

@coolsoftwaretyler Sorry for the delayed response, I was on vacation for quite a while...

I would be glad to help, although I am definitely not an "Expert" programmer by any stretch.

Here is my thoughts:

1. types.safeReference should be removed as an API altogether. If users want a maybeNull reference, then let them wrap the reference in maybeNull themselves. As it stands right now, I don't think types.safeReference has any unique value.
2. Remove the acceptsUndefined option altogether because that is handled by the maybeNull wrapper just like the rest of the MST API.
3. types.reference should gracefully handle both situations: when a reference becomes invalidated and it should handle a badReference. This can be done by either creating a new onBadReferenceHit callback or combining it with the onInvalidated API. (I prefer the combination approach because most apps will handle these situations exactly the same.
4. Add a event.removeParentFromStore() option and keep the event.removeRef()

Doing all of the above would make this whole API make sense, handle all the error cases, and provide the user with a way to handle either situation.

Curious as to your thoughts on the path mentioned above?

[coolsoftwaretyler]

Hey @ChristopherGabba - all good! No need to be an expert programmer at all. If you're energized about this, all it really takes is some time and follow-through.

I think you've got a good start here, but I'd like to reframe the direction a little, primarily to avoid breaking changes, which will allow us to move more quickly and get feedback from other users. Here's my response to your items, and then a recommendation from my side of things.

Response to the proposal

1. Removing types.safeReference would be a breaking change, so I'd like to table that discussion for a while. As we do this work, we may find there are uniquely important things about that API and decide to keep it. If we decide to eventually remove it, we'll want to go through some deprecation steps before actually removing it.
2. Similar to the first point: this is a breaking change and I don't know the full scope of its implications. I'd rather wait on this until we've explored alternatives fully, and if we decide to remove it, take a slower path through deprecation before strictly removing it.
3. I love this idea. I think we should start there. If we can keep the initial changes as entirely additive and opt-in, then we can merge it with any major version and ask for people to get us feedback with minimal risk.
4. MobX-State-Tree as a library doesn't really have a concept of "stores". That's a convention that many users have come up with, but I would rather not tie our API to the concept in any way. I do wonder if we could make the removeRef behave a little more like getParent, which takes an optional level argument and goes that number of levels up. We could maybe create a parameter for that and allow users to remove the ref and then n parents above it. I am less certain about this, and I'd like to tackle it after we work on the reference improvements. Maybe a better solution will become apparent.

Next steps

Ok, so if you're still down to work on this, I'd love to focus on the reference enhancements first. Here's an outline of steps for you as a starting point. Feel free to pick and choose what makes sense here.

1. Fork MST to your own GitHub account
2. Spin up the dependencies, make sure you can run tests and see things happening in your local repo as expected.
3. Totally optional: Once you've got the code base running for yourself, send me an email and let's coordinate a synchronous meeting. I can give you a crash course on the quirks of the code base. If you don't need or want this, all good.
4. Plan out your new API changes and submit a PR that has new tests to exercise and demonstrate the functionality, and make sure existing tests pass.
5. Once we get to that PR, I'll give an initial review with any feedback I have.
6. After we incorporate that feedback, I'll ask you to write some documentation and update existing documentation with the changes
7. We'll get it merged into whatever release makes sense, and post about it on socials and stuff.

After that, we'll see how the community responds, and identify any new changes to make afterwards (like improving the removeRef callback experience.

If it sounds good, I'll keep an eye out for your email. Have a great day!