kubk / mobx-log

A logger + Redux devtools for Mobx 6+
MIT License
55 stars 6 forks source link

mobx-log with Redux Devtools does not properly set state in the RD log entry for observable nested objects #43

Closed vaughnkoch closed 10 months ago

vaughnkoch commented 11 months ago

Hi, thank you for building this useful library. :)

I've been playing with Redux Devtools and mobx-log, see below. Each action has an entry in RD, which is very useful. However, the state associated with each action seems to depend on what's passed in:

Here's my store:

import { makeAutoObservable } from 'mobx'

import { makeLoggable } from 'mobx-log'

class MyStore {
  // All 3 properties are observable from makeAutoObservable
  todos: any = null
  foo = 0
  bar = 0

  constructor(rootStore) {
    makeAutoObservable(this)

    // mobx-log
    makeLoggable(this)
  }

  setTodos = (todos) => {
    this.todos = todos
  }

  incrementFoo = () => {
    this.foo = this.foo + 1
  }

  incrementBar = () => {
    this.bar = this.bar + 1
  }

  // Normally this would be an API call with */yield instead of async/await.
  // This is called externally by a React useEffect.
  fetchData = () => {
    // First action called - results in "MyStore@MyStore@1.fetchData" and the state shown below this code
    this.setTodos({
      // 'abc': { id: 'abc', firstName: 'First', lastName: 'Last' }, // Does not work
      'def': { id: 'def' },  // Does not work
      // 'def': 'xyz',  // Works
      // Blank object: Works
    })

    this.incrementFoo()

    this.incrementBar()
  }

}

export default MyStore

I'm using the following:

"mobx": "6.10.2",
"mobx-log": "2.2.1",
"mobx-react-lite": "4.0.5",
Redux Devtools: 3.1.3

Actual:

// In the "MyStore@MyStore@1.fetchData" entry:
{
  todos: null,
  foo: 0,
  bar: 0
}

Expected:

{
  todos: { 'def': { id: 'def' } },
  foo: 0,
  bar: 0
}

If I've missed some configuration or other gotcha, please let me know.

kubk commented 11 months ago

Hi, I’ve copied a part of your code with a nested object, and the issue isn’t reproducible for me.

Screenshot 2023-11-08 at 18 08 06

Code:

Screenshot 2023-11-08 at 18 10 56

May I ask for a simple GitHub reproduction as well as screenshots from the devtools? To avoid setting up everything from scratch, you can fork the repo, go to the example folder and modify the demo project. Use npm i && npm run start to set it up: https://github.com/kubk/mobx-log/tree/master/example

I use this subproject to test the library

vaughnkoch commented 10 months ago

Thanks for investigating. I forked the repo, modified stopwatch-store.ts, and was able to repro, although the behavior seems slightly different.

Commit in my fork: https://github.com/vaughnkoch/mobx-log/commit/e13d240e9a91d6f842bb2147e7b086113f4c953e

I labeled the scenarios A through D below. I also attached a zipfile with the screenshots.

mobx-log screenshots - Issue 43.zip

const data = {
  // A: Does not work.
  // todos are populated at StopwatchStore@nextStep,
  // expected at setTodos or fetchData.
  'abc': { id: 'abc', firstName: 'First', lastName: 'Last' },

  // B: Does not work.
  // todos are populated at StopwatchStore@start,
  // expected at setTodos or fetchData.
  // 'def': { id: 'def' },

  // C: Works.
  // todos are populated at StopwatchStore@fetchData,
  // although not at setTodos.
  // 'def': 'xyz',

  // D: Works.
  // todos are populated at StopwatchStore@setTodos.
  // Blank object
}
kubk commented 10 months ago

@vaughnkoch Thanks. Try installing an experimental fix npm i mobx-log@beta. If NPM can't locate that version - clear the cache first via npm cache clean --force

vaughnkoch commented 10 months ago

Hi @kubk, I tried mobx-log@beta (mobx-log@2.2.2-beta.1), and it appears to fix the problem, thanks! When do you think this new fix will be available in non-beta? Also, I'd be curious to know what the fix was.

kubk commented 10 months ago

@vaughnkoch 2.2.3 is ready. Thanks for reporting the issue.

I had to use setTimeout to delay the creation of the store snapshot. The previous implementation only utilized spyReportStart/spyReportEnd, and it seems like it wasn’t enough: https://mobx.js.org/analyzing-reactivity.html#:~:text=report%2Dend-,spyReportEnd,-%3Dtrue%2C%20time%3F%20(total