improved redux dev tools integration, add allParentIds to middleware events

[xaviergonz]

Closes #750

This PR:

• Adds allParentIds (array of ids from root to (excluding) current id) to middleware events
• Adds supports for flows in redux dev tools by showing them like:
  • [root].someAsyncAction (0) (code run until the first yield)
  • [root].someActionAction (1) (code run from first yield to second yield)
  • [root].someActionAction (2) (code run from second yield to third yield, etc...)
• Shows when an action / async action throws an exception by showing (also works for flows)
  • [root].someAction -error thrown-
• Shows subactions when called from actions (also works for flows)
  • [root].someAction >>> [root/child].someSubAction
• Adds unit tests for redux dev tools middleware
• As seen before now actions include the path from the root model of the object where the action is being invoked (e.g. [root/myArray/0].someAction)
• Also, inside the action object now has two properties: args: { "0": arg0, "1": arg1... } and targetTypePath: "MyModel/ChildModel" (or in the case of subactions MyModel >>> MyModel/ChildModel)

The connectReduxDevtools function now supports a third argument with options:

• logIdempotentActionSteps: true by default due to possible performance penalty because of the internal usage of onPatch. When set to false it will skip reporting of actions and flow action "steps" that do not end up in an actual change in the model (except when an error is thrown), thus reducing the amount of noise in the logs.
• logChildActions: false by default. When set to true it will report actions that are executed inside a root actions. When set to false it will not.

Not compatible with PR #1032

[xaviergonz]

@bourquep care to test it and let me know how it works for you? Make sure to add

{ skipIdempotentActionSteps: true  }

as third argument of connectReduxDevtools to skip actions / flow steps that don't actually generate a change in the model if desired.

[philipbjorge]

Is there a way to test this, other than cloning the repo down locally?
AFAIK, yarn can't install subdirectories from a git remote?

[bourquep]

The way I do it:

• Clone
• yarn build in the clone root
• Update your package.json to point to a relative path reference (e.g. "mst-middleware": "../../mobx-state-tree/packages/mst-middleware")
• Don't forget to update both mobx-state-tree and mst-middleware to a local reference

[bourquep]

@xaviergonz In my refreshAllConfigs async action, I applySnapshot on an array node and let mst do its reconciliation magic. This array contains a few hundred nodes, and most of the time the snapshots being applied are identical. It performs well.

But it makes the Redux devtools perform quite poorly with this PR, as it seems to interpret every single applySnapshot as an action. The devtools' UI become unresponsive and eventually crash:

[bourquep]

Oh, I had not pulled your latest commits to this branch. The output is a little different, but the behavior is the same (unresponsive then crash)

[xaviergonz]

@bourquep care to test it using { skipIdempotentActionSteps: true } as third argument for connectReduxDevtools?
It should avoid logging "actions" that do not end up in a change

[bourquep]

@xaviergonz That is how I have been testing it

[xaviergonz]

hmm, care to create a small piece of code to reproduce what refreshAllConfigs is doing?

[xaviergonz]

I might also create an option like "showSubactions: boolean" so that inner actions are not reported

[bourquep]

Hmm, I can give it a shot later today

[xaviergonz]

Thanks!

[xaviergonz]

@bourquep I just added:

• logIdempotentActionSteps: true by default due to possible performance penalty because of the internal usage of onPatch. When set to false it will skip reporting of actions and flow action "steps" that do not end up in an actual change in the model (except when an error is thrown), thus reducing the amount of noise in the logs.
• logChildActions: false by default. When set to true it will report actions that are executed inside a root actions. When set to false it will not.

so you may want to try it with logChildActions to false (the default) and maybe logIdempotentActionSteps to false (if that still crashes try true instead)

hopefully the logChildActions: false will not log the applySnapshot subactions part of the refreshAllConfigs root action

[bourquep]

Yes sorry I did not have time to test this any further but I will soon.

…

Le 16 oct. 2018 à 05:24, Javier Gonzalez ***@***.***> a écrit : Waiting for feedback from @bourquep I guess — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bourquep]

With the default values ({ logIdempotentActionSteps: true, logChildActions: false }), it works much better now. I do however have some noise because of the same action being logged more than once even when the state doesn't change, but this is of course to be expected with logIdempotentActionSteps=true.

If I set logIdempotentActionSteps = false, then the result is as expected: only actions which result in new state are logged. However, some state-changing actions are missing from the log. For example, this action is missing:

{
  type: '[root/configStore/configs/5b6db24ef5814e0013ec12e7/contents/5bc48cc6ff0b1f0010727e0f].toggleState (0)',
  targetTypePath: 'RootStore/SchoolYearConfigurationStore/map<string, late(function () { return __WEBPACK_IMPORTED_MODULE_4__Models_SchoolYearConfiguration__["a" /* SchoolYearConfigurationModel */]; })>/SchoolYearConfigurationModel/map<string, late(function () { return __WEBPACK_IMPORTED_MODULE_5__ContentDefinition__["a" /* ContentDefinitionModel */]; })>/ContentDefinition',
  args: {}
}

For context:

export const SchoolYearConfigurationModel = types
  .model('SchoolYearConfigurationModel', {
    id: types.identifier,
    // A bunch of attributes omitted
  })

  // Other data related to a config
  .props({
    contents: types.map(types.late(() => ContentDefinitionModel))
  });

export const ContentDefinitionModel = types
  .model('ContentDefinition', {
    id: t.identifier,
    // A bunch of attributes omitted
  })

  .actions(self => {
    const _actions = {
      toggleState() {
        if (self.state === ContentState.CANCELLED) {
          return;
        }

        let newState = self.state === ContentState.ACTIVE ? ContentState.COMPLETED : ContentState.ACTIVE;
        self.state = newState;
      }
    };

    return _actions;
  });

In short:

• SchoolYearConfiguration has a map of late ContentDefinition models
• ContentDefinition has a non-async toggleState action
• The toggleState action is logged when logIdempotentActionSteps = true
• The toggleState action is not logged when logIdempotentActionSteps = false

But at this point, I don't really mind leaving logIdempotentActionSteps to true and having some extra actions in the log, because the rest works pretty much as expected and doesn't crash Chrome anymore. :)

[bourquep]

Small request: when the action name is long, such as [root/configStore/configs/5b6db24ef5814e0013ec12e7/contents/5bc48cc6ff0b1f0010727e0f].toggleState (0) in my example above, it gets truncated in DevTools and the actual action name (toggleState) is not visible. Adding a carriage-return, if possible, might help alleviate this.

[xaviergonz]

sadly redux-dev-tools eats ¡\n' for breakfast
what could be done is reversing how it shows
toggleState @[root/...] (0)

or adding a space before the dot so it shows on the next line if there's not enough space

[root/...]
.toggleState (0)

any preference?

I'll try to use your sample to see if I can mimic your missing action btw

[bourquep]

@xaviergonz Slight preference for the second option, adding a space before the dot so it shows on the next line. But really, both would work for me.

Thanks for looking into the missing action. Let me know if you need more context.

[xaviergonz]

@bourquep is state volatile by any chance, or just a string in the props?
also, what is the default state?

[xaviergonz]

I tried this unit test

test("bourquep", () => {
    const SchoolYearConfigurationModel = types.model("SchoolYearConfigurationModel", {
        id: types.identifier,
        contents: types.map(types.late(() => ContentDefinitionModel))
    })

    const ContentDefinitionModel = types
        .model("ContentDefinition", {
            id: types.identifier,
            state: "active"
        })

        .actions(self => {
            const _actions = {
                toggleState() {
                    if (self.state === "cancelled") {
                        return
                    }

                    let newState = self.state === "active" ? "completed" : "active"
                    self.state = newState
                }
            }

            return _actions
        })

    const store = SchoolYearConfigurationModel.create({
        id: "100",
        contents: { one: { id: "200" } }
    })

    devTools = mockDevTools()
    const devToolsManager = { connectViaExtension: () => devTools, extractState: jest.fn() }
    connectReduxDevtools(devToolsManager, store, {
        logIdempotentActionSteps: false,
        logChildActions: false
    })

    store.contents.get("one")!.toggleState()
    expect(devTools.send.mock.calls).toMatchSnapshot()
})

and it properly generated this

    Object {
      "args": Object {},
      "targetTypePath": "SchoolYearConfigurationModel/map<string, late(() => ContentDefinitionModel)>/ContentDefinition",
      "type": "[root/contents/one].toggleState",
    },
    Object {
      "contents": Object {
        "one": Object {
          "id": "200",
          "state": "completed",
        },
      },
      "id": "100",
    },

Am I missing something?

[bourquep]

State is an enum, expressed as a custom type:

state: ContentStateEnum

...

const ContentStateEnumDef = t.custom<number, ContentState>({
  name: 'ContentStateEnumDef',
  fromSnapshot(value: number): ContentState {
    return ContentState[ContentState[value]];
  },
  toSnapshot(value: ContentState): number {
    return value;
  },
  isTargetType(value: number): boolean {
    return value in ContentState;
  },
  getValidationMessage(value: number): string {
    if (value in ContentState) {
      return '';
    }
    return value + ' is not a real value of ContentState.';
  }
});

[xaviergonz]

test("bourquep", () => {
    enum ContentState {
        Active,
        Cancelled,
        Completed
    }

    const ContentStateEnumDef = types.custom<number, ContentState>({
        name: "ContentStateEnumDef",
        fromSnapshot(value: number): ContentState {
            let cs = ContentState as any
            return cs[cs[value]]
        },
        toSnapshot(value: ContentState): number {
            return value
        },
        isTargetType(value: number): boolean {
            return value in ContentState
        },
        getValidationMessage(value: number): string {
            if (value in ContentState) {
                return ""
            }
            return value + " is not a real value of ContentState."
        }
    })

    const SchoolYearConfigurationModel = types.model("SchoolYearConfigurationModel", {
        id: types.identifier,
        contents: types.map(types.late(() => ContentDefinitionModel))
    })

    const ContentDefinitionModel = types
        .model("ContentDefinition", {
            id: types.identifier,
            state: types.optional(ContentStateEnumDef, 0)
        })

        .actions(self => {
            const _actions = {
                toggleState() {
                    if (self.state === ContentState.Cancelled) {
                        return
                    }

                    let newState =
                        self.state === ContentState.Active
                            ? ContentState.Completed
                            : ContentState.Active
                    self.state = newState
                }
            }

            return _actions
        })

    const store = SchoolYearConfigurationModel.create({
        id: "100",
        contents: { one: { id: "200" } }
    })

    devTools = mockDevTools()
    const devToolsManager = { connectViaExtension: () => devTools, extractState: jest.fn() }
    connectReduxDevtools(devToolsManager, store, {
        logIdempotentActionSteps: false,
        logChildActions: false
    })

    store.contents.get("one")!.toggleState()
    expect(devTools.send.mock.calls).toMatchSnapshot()
})

Object {
      "args": Object {},
      "targetTypePath": "SchoolYearConfigurationModel/map<string, late(() => ContentDefinitionModel)>/ContentDefinition",
      "type": "[root/contents/one] toggleState",
    },
    Object {
      "contents": Object {
        "one": Object {
          "id": "200",
          "state": 2,
        },
      },
      "id": "100",
    },

still looking good to me

[xaviergonz]

by any chance, maybe you are calling the action when it is in a cancelled state and therefore not generating a change in the model and that's why it is not being logged when logIdempotentActions is set to false?

[xaviergonz]

Added

• logArgsNearName: true by default. When true it will log the arguments near the action name (truncated if too long), when false it won't.

so it will show [root/child] action(123, "abc")
if args are over 64 chars it will truncate and add an ellipsis

either way they can still be accessed from "args" inside the action info

[bourquep]

maybe you are calling the action when it is in a cancelled state and therefore not generating a change in the model

No

I notice in your test case that the keys used in the map don't correspond to the object's identifier. Probably has nothing to do with the issue at hand, but mentionning it just in case...

I'll try to see if I can adapt the test case to make it behave like what I am observing.

[xaviergonz]

thanks, it would be great if you could get the unit test to fail

[bourquep]

I am unable to make the test fail. I'll have to dig deeper into my codebase, it might very well be the case that we are doing something not quite right in this action.

I suggest this PR should be merged and deployed as-is.

[xaviergonz]

thanks for trying, up to @mweststrate now then :)

[mweststrate]

NIT: EMPTY_ARRAY to avoid needles array allocations

[mweststrate]

@xaviergonz looking good, not merged yet due to conflicts

[xaviergonz]

thanks, will fix them later today. I was thinking of cutting a release after this merge. should it be minor I guess?

[mweststrate]

Yep I think so. But released the typescript stuff already 😏 Op do 18 okt. 2018 15:55 schreef Javier Gonzalez <[email protected]>:

…

thanks, will fix them later today. I was thinking of cutting a release after this merge. should it be minor I guess? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1035 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhFz5HcLw_Uiz13IHYCuENoPS8JS2ks5umIhggaJpZM4XMAyl> .