Next.js: with-mobx example global store not properly implemented?

[imsnif]

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

I'll start by saying this might be a misunderstanding of mine, but I have gone over this a few times and believe there is an issue here. My apologies if I am mistaken!

Expected Behavior

In the with-mobx example: https://github.com/zeit/next.js/tree/master/examples/with-mobx
The store is implemented both on the server side and on the client side.
As explained in the README, on the server-side a new instance is created for each page request so as not to mix between users/requests, etc. On the client side, it is a singleton so that different components/pages will have the same state and to allow us to keep a global state.
The way the example implements it, state created by the server will be propagated to the client but not to the client's store.

Since getInitialProps on first load is run on the server: https://github.com/zeit/next.js/blob/master/examples/with-mobx/pages/index.js#L7-L11
Then the isServer property will be true on first load on the client as well: https://github.com/zeit/next.js/blob/master/examples/with-mobx/pages/index.js#L15

This would mean that the store created on the client will not be a singleton, and when we navigate to other pages with Link, a new store will be created without the state data propagated from the server.

My desired (expected) behaviour would be to have some way to distinguish between client and server when creating the store even on initial page load (this is critical, to my understanding, for keeping global state between pages). I'm currently achieving this with something like:

getInitialProps ({req}) {
  // ...
  // here returning isServer as !!req would not be good
  // this is not run on the client on initial load, 
  // so in that case props.isServer would be true on the client as well
  return { myData }
}
constructor (props) {
  const isServer = typeof window === undefined
  initStore(isServer, props.myData)
}

Current Behavior

The example uses const isServer = !!req inside getInitialProps - this results in the initial server begotten state not being stored in the client store.

Context

If I want to store global state, I am only able to do that starting from the second page I navigate to using the with-mobx example. In my workaround I use (as mentioned above) const isServer = typeof window === undefined which I think is a somewhat non-native to next.js solution... if you have a better suggestion I would love to hear it!
I'm mostly opening this issue because I think I won't be the only one affected and confused by it.

[vuldin]

I think previously merged PR #2812 is related to this issue.

[imsnif]

That would do it, @vuldin ! Thanks for finding it.
I'll admit that I actually don't understand the benefit of having a store on the server side. Maybe I'm missing something, but I'd usually opt for the server not keeping any state in memory and instead just getting it from its respective state persister (db, etc.)
Maybe one of the maintainers can shed some light on this issue?

[vuldin]

@imsnif here's my attempt at solving this issue. I wanted someone else to see it before sending in a PR. There are a few changes:

• using pathname in getInitialProps to set an origin property on the store
• setting prevOrigin and origin within an intercept mobx function
• passing the store as a state variable from getInitialProps. This can be either an actual store (if getInitialProps was called client-side) or only props that can be used to instantiate the actual store (determined in each page's constructor)
• displayed prevOrigin in a div on the Page component (just above the clock)
• set the store variable inside the Store constructor to ensure that this variable always contains a copy of the most recently instantiated store.

The whole reason for introducing the origin variables (and the intercept function) is to show how previous state values are correctly being passed along to newly instantiated stores.

https://github.com/vuldin/next.js/tree/fix-mobx-store-implementation/examples/with-mobx

[vuldin]

Another note regarding the use of pathname: getInitialProps is using pathname over req.url for setting the origin variable due to req being a server-side only variable within getInitialProps. Using pathname ensures that origin is set regardless of where getInitialProps is called from.

It's possible this branch is more complicated than what it has to be to solve the problem. It's also possible that it doesn't even solve the issue you mentioned! Take a look if you have time.

[imsnif]

Hey @vuldin - thanks for taking a crack at this!
I haven't yet had a chance to take a look at the solution, I promise I will soon. But before I do, do you have a use case in which I'd need a mobx store on the backend? All of my architectural gut feeling tells me it's an anti-pattern, for the reasons I originally mentioned (edit to make it clearer: that I don't think the server should hold state outside of a persistence solution such as a db). What am I not thinking of?

[vuldin]

@imsnif The main reason for having a store on the server is to enable server-side rendering for components which require the store. Another way of saying this is to enable isomorphic components.

It may be possible to write the components in a way that doesn't require a store (yet still makes use of it when its available), but I haven't looked into this enough to know how this would work or if it would make things less complicated. My initial thought is that each component (and the project overall) would be way more complicated. I think the components would have to know much more about the state, and basically recreate their required parts of the store so it would be business as usual for them even when being rendered on the server (when the store wasn't available).

[imsnif]

Hey @vuldin - thanks for the explanation. While I prefer to render components on the server-side without the need for a store (using defaults/props or just polyfill) - other approaches are of course valid as well. :)

As for the fix - from a quick glance it doesn't look to me as if it solves the problem. Mostly due to this line: https://github.com/vuldin/next.js/blob/fix-mobx-store-implementation/examples/with-mobx/pages/index.js#L16
All the props would initially be coming from the server and so the store will be initialized with the server origin until client navigation takes over and changes the origin somehow. So, same problem...
Also, in addition - the store will be a singleton across users on the server-side.

I'm saying this though just from going over the code briefly - so if you don't think this is the case and I missed something, I'd be happy to give it a 'live' try later.

[jisaac89]

A mobx global store example would be really helpful.

I am trying to implement a night-mode flag (simple observable boolean) on my mobx store, the problem is the state does not persist when I switch routes. Doesn't this defeat the purpose of mobx or am I missing something?

** Works now, not sure what I missed? https://github.com/jisaac89/react-recipe

[John0x]

I believe a better with-mobx example would be beneficial for a lot of people. There's a lot of repeating code and problems with the current version, which is really discouraging.

[ostenning]

+1 The current example is not right.

[mogadanez]

Actually wants a way to deal with multiple stores, like in real world app
https://github.com/gothinkster/react-mobx-realworld-example-app/tree/master/src/stores

[alfredvaa]

It seems to me that the isServer variable in initializeStore in the with-mobx example is truthy on client side as well, is that correct? It is a bit confusing.

[matteing]

This is a show stopper to newbies trying next out.

[timneutkens]

@maranomynet did some improvements to the example recently, mainly the store is initialized in the _app.js constructor on the client side now, which as far as I'm reading was @imsnif's initial issue 🤔 So I guess this can be closed.

[imsnif]

Hey @timneutkens - tbh I've been out of this context for nearly a year, so I no longer have an easy way of reproducing this. I think my initial write-up has enough details to reproduce this with the new example and test to see if it makes sense?

In any case, I defer to your judgment in handling this issue. My apologies for not being able to provide more insight.

[ostenning]

I just recently identified an issue with the current implementation of this example.

_app.js:

 constructor(props) {
    super(props)
    const isServer = typeof window === 'undefined'
    this.mobxStore = isServer
      ? props.initialMobxState
      : initializeStore(props.initialMobxState)
}

The issue is that when isServer is true, this.mobxStore === props.initialMobxState - This is OK in theory, but, at least for me, methods aren't being serialized apart of the initialMobxState object.

For example:

class MyStore {
   constructor() {
      this.defaultState();
   }
   defaultState() {
     // This method does not get serialized and is undefined
  }

  @get something() {
     // This method does not get serialized and is undefined
  }

  somethingElse = () => {
    // Does get serialized
  }
}

One solution I had was to re-initialize the store and hydrate on the SSR also:

_app.js:

 constructor(props) {
    super(props)
    const isServer = typeof window === 'undefined'
    this.mobxStore = initializeStore(props.initialMobxState)
}

But im not sure if this is the best approach.

[maranomynet]

@osphost my understanding is that methods (getters/computeds) should not be serialized and sent over the wire - as they can be derived from the static values.
So this would be mobx serialization behaving as intended.

[maranomynet]

@osphost on the server initializeStore() has been run only a few ticks earlier (here) and props.initialMobxState is literally that result.

The "serialization" only happens when the state is written into the rendered HTML to be passed over the wire to the browser.

In the browser, the initial getInitialProps() call is skipped, so the incoming props.initialMobxState is the static (serialized) store data which therefore needs to be passed to initializeStore() to fire up all the getters and other niceties.

So if you're finding yourself in the need to rerun initializeStore() on the server - then my guess would be that the problem lies in your mobx store class and/or how you initialize it.

[maranomynet]

@timneutkens Actually, I believe the OP's issue might have been solved with #4705

[timneutkens]

@maranomynet yeah exactly what I was thinking, the issue is quite old, so I assumed it was fixed in one of the 3-4 PRs we had to the example 😄 Thanks for checking it out 🙏