infinitered / ignite

Infinite Red's battle-tested React Native project boilerplate, along with a CLI, component/model generators, and more!
MIT License
17.36k stars 1.38k forks source link

MobX State Tree Incompatible with React Navigation v7: [MobX] Dynamic observable objects cannot be frozen #2753

Closed ChristopherGabba closed 3 days ago

ChristopherGabba commented 3 weeks ago

Describe the bug

In the ignite demo app, passing an observable MST model as a param to another screen via React Navigation causes a crash when using new React Navigation v7.

Steps to reproduce:

  1. Upgrade to React Navigation v7 by using the following diff and install:

    Screenshot 2024-08-22 at 1 49 23 PM
  2. Create a dummy screen (that is observable) and pass an episode (from EpisodeModel) as a param to the dummy screen. Example: navigation.navigate("DummyScreen", { episode }).

  3. Tap the button to navigate.

The app will crash and throw the Error above: [MobX] Dynamic observable objects cannot be frozen.

This appears to be introduced as an MST error in the React Navigation v7.

Ignite version

9.8.2

Additional info

System platform darwin
arch arm64
cpu 10 cores Apple M2 Pro
directory ReelFeelApp /Users/christophergabba/Documents/ReelFeelApp

JavaScript (and globally-installed packages) node 22.6.0 /opt/homebrew/bin/node npm 10.8.2 /opt/homebrew/bin/npm
@aws-amplify/cli-internal 12.12.0
@aws-amplify/cli 12.12.1
@react-native-community/netinfo 9.4.1
eas-cli 10.2.4
expo-cli 6.3.10
firebase-tools 11.24.1
n 9.1.0
node-gyp 10.0.1
node 20.6.0
npm 10.7.0
pod-install 0.2.2
react-native-spinkit 1.5.1
reel-feel -
reelfeelwebsite 0.0.0
typescript 5.4.5
yarn 1.22.22
yarn 1.22.22 /opt/homebrew/bin/yarn create-amplify 1.0.1
pnpm - not installed
bun - not installed
expo 51.0.28 managed

Ignite ignite-cli 9.9.0 /Users/christophergabba/.npm/_npx/e31027f3785124a8/node_modules/.bin/ignite
ignite src build /Users/christophergabba/.npm/_npx/e31027f3785124a8/node_modules/ignite-cli/build

Android java 1.8.0_371 /usr/bin/java
android home - /Users/christophergabba/Library/Android/sdk

iOS xcode 15.4

frankcalise commented 3 weeks ago

Can you post a reproducible demo to GitHub somewhere? It'll be easier for @coolsoftwaretyler and I to look at it

coolsoftwaretyler commented 3 weeks ago

Ooh that's not fun!

I will definitely work to investigate, although it looks like that error is coming from MobX itself (the two libraries prefix our error messages differently).

Either way, I am happy to check it out, but it's possible we'll need to go a little deeper in the stack to fix this one.

ChristopherGabba commented 3 weeks ago

@coolsoftwaretyler @frankcalise Good afternoon gents, thanks for looking into this. Here is a perfect 100% reproducible demo using the basic ignite app: https://github.com/ChristopherGabba/BugHuntingForFrankAndTyler

Clone the repo and run npx eas build --profile development:device --platform ios

  1. Sign in by hitting the Log in button
  2. Click on the Podcasts Tab
  3. You'll see a list of podcasts
  4. Tap any podcast.

All I'm doing is passing the tapped episode MST object as a prop to a new screen I call TestScreen and MobX throws that error.

@frankcalise You'll also notice that Typescript is complaining about AppStackParamList and DemoTabParamList as well as the AppNavigator ref field. If you could look into that as well, that would be awesome. Clearly has to do with react navigation v7 because Typescript doesn't complain at all until I upgrade.

Thanks again guys!

frankcalise commented 3 weeks ago

@ChristopherGabba I see, I'm not familiar with v7 yet but I probably wouldn't bother passing the model around in the event there are props that could give trouble

I'd just pass the id and then look it up on the detail screen

// detail screen
const episode = episodeStore.episodeById(id as string)

// EpisodeStore
 .views((store) => ({
    get episodesForList() {
      return store.favoritesOnly ? store.favorites : store.episodes
    },

    hasFavorite(episode: Episode) {
      return store.favorites.includes(episode)
    },

    episodeById(guid: string) {
      return store.episodes.find((episode) => episode.guid === guid)
    },
  }))
ChristopherGabba commented 2 weeks ago

@frankcalise good advice, I guess that is very logical, and I'll do that in my own app. Just never had a problem until I upgraded to v7.. kind of interesting.

I've been passing a model as a prop in v6 for about a year now and never had an issue at all. Is there a reason as to why this is a bad idea? I guess what could go wrong? For my own learning lol

frankcalise commented 2 weeks ago

@coolsoftwaretyler would have a better idea than me on the types.frozen piece. I just try to think of it in terms of future proofing, like moving to expo router as an example. When I navigate I don't want to pass a bunch of information that would need to be serialized in the URL.

I'll read up on the v7 changes to see if I can understand it better.

Oh wait I do know that the navigate method changed. I think there is navigateDeprecated or something to use the v6 version. Perhaps you're running into that?

coolsoftwaretyler commented 2 weeks ago

Still waiting for EAS build on my free account, but I will say that React Navigation has always recommended against using non-serializable values in navigation params, even in v6.

While an instance of Episode is in fact serializable (in that you could call getSnapshot on it and get a serialized representation of it), the actual observable instance of Episode is not a serialized value in and of itself at that moment.

I wonder if React Navigation v7 is starting to enforce this and freezing the values. It looks like this commit maybe started doing that: https://github.com/react-navigation/react-navigation/commit/2a5721b3a3560b76192d3aa46a4ea3be9a50db7d

And MobX is unhappy because it is trying to prevent Object.freeze on a dynamic object. See preventExtensions: https://github.com/mobxjs/mobx/blob/62d994d32d89500cf9cf10e0ee76e6fb50a44f41/packages/mobx/src/types/dynamicobject.ts#L77, which throws error 13: https://github.com/mobxjs/mobx/blob/62d994d32d89500cf9cf10e0ee76e6fb50a44f41/packages/mobx/src/errors.ts#L25

So it seems this is a pattern that React Navigation used to discourage, but is now enforcing by deep freezing parameters passed to it, and that deep freeze behavior is causing a MobX error.

Alternatives you could consider:

  1. Pass a reference to the Episode and then find it in the store and get the observable instance in the component that accepts that param (like Frank's idea)
  2. Pass a snapshot, or use .toJS on the object itself, and then create a new model based on that snapshot
  3. If you don't need observability and all the MST views and stuff, and just want to "view" the data of the model, you could just render the snapshot like they're regular React props.

I would probably recommend option 1 since it feels more correct, and option 2 would end up instantiating a bunch of model instances that would be disconnected from your state. Option 3 seems like you'd be losing all the value of observable state, but it may also be sufficient.

ChristopherGabba commented 2 weeks ago

Hey guys, can confirm going with Franks Option 1 is the right approach, good lesson learned for me. Everything seems to be working well! I appreciate you both looking into this so quickly, so helpful.

@frankcalise Looks like TS is just complaining about the refs (the example app I have shows it). I've tried debugging it but can't seem to figure out why Typescript doesn't like it.

Screenshot 2024-08-23 at 5 16 32 AM