[BUG] Mutations list is empty in SSR environment

[karol-maciaszek]

Summary

The list of mutations is empty when running SSR-version of Overmind and those conditions are true:

• in a non-productive environment (NODE_ENV !== 'production' specifically)
• using action which modifies state asynchronously
• .hydrate() is also called asynchronously

Reproducible example:

const { createOvermindSSR } = require('overmind');

async function main() {
    const overmind = createOvermindSSR(
    {
            actions: {
                increment: async ({ state }) => {
                    // if you comment out following line, mutations list is correct
                    await new Promise(r => setTimeout(r, 1));
                    state.a++;
                },
            },
            state: { a: 0 },
        }
    );

    await overmind.actions.increment();

    // if you comment out following line, mutations list is correct
    await new Promise(r => setTimeout(r, 1));

    console.log(overmind.hydrate());
}

main().catch(console.error);

Expected result

[
  {
    method: 'set',
    path: 'a',
    args: [ 1 ],
    delimiter: '.',
    hasChangedValue: true
  }
]

Actual result:

[]

Environment:

Overmind version: 25.0.2
Node version: 12.18.3

Some leads

These are some findings of my investigation, they may be inaccurate / feel free to ignore it :)

The flow goes like that: when you set state (either directly or via action) then this overmind code is executed: https://github.com/cerebral/overmind/blob/next/packages/node_modules/overmind/src/index.ts#L546

If you are not in production mode it jumps to the else clause: https://github.com/cerebral/overmind/blob/next/packages/node_modules/overmind/src/index.ts#L593

After doing some stuff, it schedules a flush of mutations here: https://github.com/cerebral/overmind/blob/next/packages/node_modules/overmind/src/index.ts#L677

Flush is basically: https://github.com/cerebral/overmind/blob/next/packages/node_modules/proxy-state-tree/src/index.ts#L131 which executes getMutations() here: https://github.com/cerebral/overmind/blob/next/packages/node_modules/proxy-state-tree/src/MutationTree.ts#L42

What is interesting, getMutations() its also clearing them https://github.com/cerebral/overmind/blob/next/packages/node_modules/proxy-state-tree/src/MutationTree.ts#L45

[christianalfoni]

@karol-maciaszek Hi there!

The Overmind SSR implementation is intended to run synchronously because of this reasoning:

Your actions are built to run on the client, so you really do not want them to run on the server as they can have browser specific dependencies and logic. This is why OnInitialize does not run either on the server.

If you need to run asynchronous logic, for example to populate the state store, you run that with specific server side logic first and then update the state of the Overmind SSR instance synchronously.

That said, there has been talk about allowing client/server logic, but yeah... I am not convinced it is better. Cause the server is such a different environment. But yeah, totally open for further discussions here 😄

[karol-maciaszek]

@christianalfoni Thank's for your reply!

I’d say that if Overmind runs actions server-side then they should work fully without distinguishing whether this is an async or sync function. The other option would be to not run them at all so it is clear to the developer that stuff should be computed elsewhere when in server environment. I vote on keeping this issue open.

[christianalfoni]

Yeah, I agree we should at least throw an error if this occurs. But I am curious. Why did you need to do something async in the first place? I have seen cross client/server logic in frameworks, like Meteor, but unsure how appealing they are. React is also synchronous, though I think I saw something that they are looking at ServerComponents now which I believe renders async on the server.

But yeah, I just need concrete use cases to defend adding features as they add maintenance as well. So need it to be worth it 😄

[karol-maciaszek]

I hope the following pseudo-code explains my use-case.

express

// step #1
.use(async (req: express.Request, res: express.Response & { overmind: OvermindSSR<...> }, next: express.NextFunction) => {
 ...common initialization logic...
 ...async action called...
 next();
})

// step #2
.use(vhost(vhostRegex, async (req: express.Request, res: express.Response & { overmind: OvermindSSR<...> }, next: express.NextFunction) => {
  ...vhost specific logic (needs data from step #1)...
  next();
}))

// step #3
.use(async (req: express.Request, res: express.Response & { overmind: OvermindSSR<...> }, next: express.NextFunction) => {
  if (!req.vhost) {
    ...non-vhost specific logic (needs data from step #1)...
  }
  next();
})

// step #4
.use(async (req: express.Request, res: express.Response & { overmind: OvermindSSR<...> }, next: express.NextFunction) => {
 ...dump mutations to HTML...
})

As you can see there is a common logic in step #1 which also executes async actions (e.g. reporting). Then either step #2 or #3 is executed, depending on whether it is a call to vhost or direct. Both #2 and #3 are async in relation to #1 and #4.

I hope I made the use-case clear. The logic of my app could be rewritten and adjusted so it computes everything and then stores it in Overmind. But I still believe it is buggy behavior and I'm not the last person hitting this.