v2 prerelease feedback

[Lab3l]

I'm struggling with async thunk actions and their actions not being reflect on rendered side.
If I have a thunk with multiple dispatches, e.g.
export const thunkadoo = async () => { await dispatch('general.getValue1') await dispatch('general.getValue2') }
and inside getValue1 and getValue2 are async actions that take x-amount of time each, either the thunk is not respecting the awaits or dispatching an action is not updating the store values correctly on subscribed windows. I've hackishly "solved" this by wrapping my dispatched with an async wait when dispatching async actions, but this is inevitable creating more and more problems now that we're closing in on production use.
To expand more on the written example, let's say my getValue1 and getValue2 update general.counter to a random integer. The visible react renderer will not be showing the latest value getValue2 has set and instead will not display any changes or will display getValue1's set value instead of the later getValue2's set value

[goosewobbler]

@Lab3l Presumably this is in the renderer process, or are you using thunks in the main process via the dispatch helper? Or both?

When you say "inside getValue1 and getValue2 are async actions" - what does that mean? are you dispatching nested thunks?

Any detailed, specific example would be helpful, or preferably a minimal reproduction repo that illustrates the exact problem you are facing.

[Lab3l]

This happens with dispatching thunks in renderer process via useDispatch(window.zubridge)
Not dispatching nested thunks, here's example code showing what I mean
export const thunkadoo = async () => { await dispatch('general.getValue1') await dispatch('general.getValue2') }
General store handlers:
export const handlers = (store: Store) => { return { 'general.getValue1': async () => { const value = await fetch('someurlwhichtakes150mstoload') store.setState({ value }) }, 'general.getValue2': async () => { const value = await fetch('someurlwhichtakes1500mstoload') store.setState({ value }) },
When calling await dispatch(thunkadoo), the respectfully set final values (getValue2) are not being reflected on my subscribed windows. Seems to be somewhat "random", sometimes I'm seeing the correct values in my window, sometimes only the value being set in getValue1 will be shown, or sometimes neither. I'll make a reproducible repo showing the problem

[goosewobbler]

Ok I think I might have an idea about what's going on here. I'm making a breaking change to the interface (#89) so will get some v2 prebuilds out later and the fix for this should be in there.

Also just a note, you don't have to pass handlers into useDispatch any more if you are using window.zubridge - passing handlers here is only really useful if your handlers are exposed to the renderer process via a different namespace.

[Lab3l]

So does that mean you don't need the reproducible repo? This is a deal breaker at the moment for us as the hackish "fix" was extremely unreliable on our production testings and provided a really bad user experience so I'm eagerly waiting for the prebuilds to try them out!

[goosewobbler]

No, I think I've worked out where the issue lies - basically we were only testing trivial thunks and there needs to be logic around ensuring the sequencing in async thunks where you are performing multiple dispatches. I'm adding a logging component to test sequencing, adding E2E tests for this case which should break and then applying a fix.

[goosewobbler]

@Lab3l I've released the first prerelease, you can install it via the next NPM tag. The fix for this should be in there though it is not E2E tested yet. Let me know if it fixes the issue, development for v2 is continuing in #94.

[Lab3l]

Fetched the 2.0.0@next, but I'm now getting errors from my preload script with that version. VM4 sandbox_bundle:2 Error: module not found: uuid at preloadRequire (VM4 sandbox_bundle:2:151308) at <anonymous>:4:14 at runPreloadScript (VM4 sandbox_bundle:2:151972)
And I'm assuming 'cos of that, after these errors I'm also getting in console Uncaught Error: Zubridge handlers not found in window. Make sure the preload script is properly set up. at createHandlers (@zubridge_electron.js?v=2d71b00e:70:11) at createUseStore (@zubridge_electron.js?v=2d71b00e:75:38) at index.ts:20:25
Edit: Extra clarification, 1.3.0 works fine with the same code

[goosewobbler]

I think it is a bundling issue, the E2Es aren't set up to catch these yet. I released 2.0.0-next.1 though due to a CI workflow issue it seems these have been pushed to npm under the latest tag instead of next, very annoying.

I will have to release a 2.0.0-next.2 under next and then 1.3.1 as latest to fix this. In the meantime, please try out 2.0.1

UPDATE: 2.0.0-next.2 has been released with the correct tag on NPM, it's the same as 2.0.1.

[Lab3l]

I assume that's 2.0.0-next.1 & 2.0.0-next.2? If so, facing the same errors in console with these

[goosewobbler]

Yes that's correct, I updated my comment. Well, looks like it's more involved than I thought...

[goosewobbler]

@Lab3l Ok one more release before I have to get on with other things, try 2.0.0-next.3. If it doesn't work I'll try get to this later.

[Lab3l]

Unfortunately still happening with 2.0.0-next.3 as well

[goosewobbler]

Ok last attempt - AI led me down the wrong path but I verified locally that uuid wasn't being bundled correctly, now it looks like it is. Try 2.0.0-next.4

[Lab3l]

Still the same issue with 2.0.0-next.4 as well

[goosewobbler]

Hmmm well, I spun up a new electron-vite repo and it seems to work there. Are you sure you have that version?

[Lab3l]

Coming out of my tunnel vision, I just realized that here's a simpler way to reproduce the problem. Even dispatching an async action from renderer will cause the same behavior. So basically having a function run in renderer via from a button click
const handleClick = async () => { await dispatch('general.setValue', 2) window.alert('!') }
Granted that there's a handler 'general.setValue': async (valueToSet) => { await sleepy(2500) setState({ value: valueToSet }) },
will cause this:
Expected behavior

Clicking the button
Getting a window alert after the general.setValue resolves, i.e. ~2500ms later

Actual behavior

Clicking the button
Getting a window alert immediately due to the general.setValue not being awaited to resolve

[goosewobbler]

Random gym thought - I removed the auto subscription from the bridge, so you have to instantiate the bridge first then use the returned subscribe method to subscribe windows now. Not sure if I mentioned it earlier or if you've already updated your repo but not subscribing windows isn't going to help matters, haha.

[goosewobbler]

Coming out of my tunnel vision, I just realized that here's a simpler way to reproduce the problem. Even dispatching an async action from renderer will cause the same behavior. So basically having a function run in renderer via from a button click const handleClick = async () => { await dispatch('general.setValue', 2) window.alert('!') } Granted that there's a handler 'general.setValue': async (valueToSet) => { await sleepy(2500) setState({ value: valueToSet }) }, will cause this: Expected behavior

Clicking the button
Getting a window alert after the general.setValue resolves, i.e. ~2500ms later

Actual behavior

Clicking the button
Getting a window alert immediately due to the general.setValue not being awaited to resolve

That's great, another E2E right there.

Looking back at this thread I see where things got confused, we've not been disambiguating between dispatches from within a thunk and from outside a thunk. I also thought of another edge case we need to test, starting a thunk from one window and then starting another one from a different window whilst the first is in process. What is the desired behaviour there? I'm thinking defer the second one until after the first one is processed, not sure if this is what happens right now. We will need to update all this logic for selective updates anyway as we want thunks and actions affecting a different part of the state to that of the "in process" thunk, to be let through.

[Lab3l]

In an ideal world the queueing system would be the best solution I think, i.e. FIFO the dispatches 🤔
Btw, I don't know if you remember, but I actually asked you a question on zutron a while back about dispatching an action from within an action (goosewobbler/zutron#79). This is why I navigated towards thunks in the first place. It just occurred to me that back then I had a working system of doing dispatches from within the actions, but the convention of my coding seemed silly which led me to ask that question from you. A brief explanation of the "system" in question:
Say we have handlers 'general.doThis', 'user.doThat'
If we're dispatching general.doThis, you can actually successfully dispatch user.doThat from within the dispatch general.doThis. You would import the actionHandlers from the store's index & dispatch the user.doThat action by await actionHandlers(store)['user.doThat']() This properly handles async dispatches and works the exact way I'd imagine thunks or dispatching from renderer should work. Granted that in this way the renderer will also not wait for the initial general.doThis async functions properly, but for example my code could be refactored to do the thunks I currently have in the aforementioned way. All the store variables are in the correct state and awaits are respected. I think my explanation is very confusing and not that clear, do you want me to give you a demo repo with that implementation, if that helps and or grants you any thoughts regarding the renderer async dispatch problems? Like I said, I just remembered this now and I'm leaning towards refactoring my code as I already tested and I can get everything to work the way I want with that implementation. The only thing that's giving me second thoughts is that in the discussion way back in december you mentioned that the handlers are not supposed to work in that way

[goosewobbler]

In an ideal world the queueing system would be the best solution I think, i.e. FIFO the dispatches 🤔

This is the current queueing system, it's a priority-based queue with specific logic around thunks (e.g. defer actions when a thunk is in flight - I implemented this after misunderstanding the issue here).

https://github.com/goosewobbler/zubridge/blob/feature/electron-v2.x/packages/electron/src/main/actionQueue.ts

Say we have handlers 'general.doThis', 'user.doThat' If we're dispatching general.doThis, you can actually successfully dispatch user.doThat from within the dispatch general.doThis. You would import the actionHandlers from the store's index & dispatch the user.doThat action by await actionHandlers(store)['user.doThat']() This properly handles async dispatches and works the exact way I'd imagine thunks or dispatching from renderer should work. Granted that in this way the renderer will also not wait for the initial general.doThis async functions properly, but for example my code could be refactored to do the thunks I currently have in the aforementioned way. All the store variables are in the correct state and awaits are respected. I think my explanation is very confusing and not that clear, do you want me to give you a demo repo with that implementation, if that helps and or grants you any thoughts regarding the renderer async dispatch problems? Like I said, I just remembered this now and I'm leaning towards refactoring my code as I already tested and I can get everything to work the way I want with that implementation. The only thing that's giving me second thoughts is that in the discussion way back in december you mentioned that the handlers are not supposed to work in that way

I think you're getting into the realms of more existential questions around side-effects, store structure and overall app architecture when using state management solutions, which is obviously not something that Zubridge should really be concerned with, given that it's basically just a neat way to get your state across the IPC boundary. One of the problems this library will have as it gets more popular is people dumping their random Zustand state issues and poor architectural decisions at my door, it's inevitable 😅

I personally came to Zustand after a long time working with (and getting frustrated by) Redux. That sounds like a bad thing, it's not - I believe Zustand is a lot easier to work with if you've already become accustomed to the more rigid, opinionated, boilerplatey way of doing things that is involved with building on Redux / RTK. Zustand is a lot more fluid and easy to use but I do have a feeling the unopinionated approach may more readily lead to footguns.

The reason why I said action handlers shouldn't dispatch further actions is this Redux architectural norm:

https://redux.js.org/style-guide/#reducers-must-not-have-side-effects

Ideally each action handler / reducer that you use should do just one thing, which is update the state in a limited way. If you want side effects and sequenced events then you typically don't do that as part of your store handlers. Your example of a handler which kicks off multiple actions is basically a thunk. Now thunks aren't special, they are just async functions which kick off a series of state actions based results to other actions and / or side effects, you can easily use the Zubridge dispatch hook and getState to roll your own:

// DIY thunk implementation
const useMyThunkDispatch = () => {
  const dispatch = useDispatch();
  const getState = useStore.getState; // Get the state getter from the store hook

  return (action) => {
    // If the action is a function (thunk), call it with getState and dispatch
    if (typeof action === 'function') {
      return action(getState, dispatch);
    }
    // Otherwise, just dispatch it normally
    return dispatch(action);
  };
};

// Usage
const myThunkDispatch = useMyThunkDispatch();
myThunkDispatch((getState, dispatch) => {
  // Your async logic here
  const currentState = getState();
  // Do something with the state
  dispatch('SOME_ACTION');
});

This is effectively what Zubridge was doing internally, at least before you opened this issue. Now it does a hell of a lot more.

With all that said, I'm not here to tell you how to structure your app, state or store handlers. Zustand is unopinionated for a reason and it's a lot more fluid than Redux / RTK, all of this is my opinion and you can do what you like. There's likely a few cool ways of using Zustand which I haven't appreciated yet, maybe also I'm overstating an architectural principal which makes sense for Redux but doesn't apply in quite the same way to Zustand. I'd recommend asking in the Zustand Discussions about best practices in this area...

Anyway to get back to this never-ending issue, I forked your repo and submitted a PR to fix some random stuff that I found and ensure your windows will be subscribed. If you could try again with this code, I'll try to work out why the E2Es are failing and also implement the new E2E as described above. Once this works we will then need to look at the behaviour around dispatching a thunk whilst another thunk is in process elsewhere.

[goosewobbler]

Got the E2Es passing locally, working through CI flakiness. Still a long way to go before v2 but I can see the end of this thunk / action handling stuff now. Did another prebuild. I'll add a section in the docs about async handlers support seeing as they seem to work fine now...

[goosewobbler]

Reworked the thunk management completely, that action queue was doing way too much so I pulled it apart. The global thunk locking seems to work well across all contexts now, you can try it out in 2.0.0-next.13. Next up is force mode which is an escape hatch for the global lock, so if you don't care about interleaved actions or simply want to manage action sequencing yourself then you can.

EDIT: Changed my mind, doing selective updates / subscriptions instead as it will likely significantly affect how the force mode is implemented.

[Lab3l]

Took a small hiatus, back at it again. I agree with what you said. I agree with what you're saying about thunks being the same as what I mentioned when dispatching actions inside reducers, no argument there :D
I tried installing your next.13, but it's giving me errors from imports `node_modules/@zubridge/electron/dist/utils/handlers.js (1:9): "debug" is not exported by "node_modules/@zubridge/core/dist/index.js", imported by "node_modules/@zubridge/electron/dist/utils/handlers.js".
file: /Users/tuisku/Documents/Workspace/02/zubridge-fixes/node_modules/@zubridge/electron/dist/utils/handlers.js:1:9

1: import { debug } from '@zubridge/core';
^
2: /**
3: * Helper function to find a case-insensitive match in an object

✘ [ERROR] No matching export in "node_modules/@zubridge/core/dist/index.js" for import "debug"`
In my master branch & your pr code when running yarn start
Edit: And naturally, I've tried removing node_modules and clearing caches before commenting

[goosewobbler]

Looks like there was some glitch in the publish workflow and 2.0.0-next.0 of @zubridge/core got released without files. Should be fixed now. I'll put 2.0.0-next.14 of @zubridge/electron up too - still a long way to go for the v2 release but I think it's feature complete now.

[Lab3l]

Without adding @zubridge/types to dependencies on next.14 I'm getting an error Rollup failed to resolve import "@zubridge/types"
After adding @zubridge/types as a dependency dist-electron/main/index.js:14 import { ThunkState } from "@zubridge/types";

[goosewobbler]

Goddamn, this is why I need to find time for #76 and some package tests 😓

[goosewobbler]

Try next.19. Fixed the types issue and another two package issues

[goosewobbler]

Converted to a discussion as I've fixed the async issue, this is now about getting v2 ready.

[goosewobbler]

I think we start #76 next and get package tests done.

[goosewobbler]

Released next.20 to handle the init case where the subscription manager isn't available yet.