klarna / electron-redux

Use redux in the main and browser processes in electron
MIT License
748 stars 95 forks source link

Discussion: renderer process stores are not main process store mere proxies #64

Open eric-burel opened 7 years ago

eric-burel commented 7 years ago

Hi, I'd just like to raise this issue because it has been bugging me for a while. In the readme, you declare that the main process becomes the Single Source of Truth, and renderer mere proxies.

I am not sure about that actually. That would be true if this lib was based on state diffing, but here, as far as I understand, you use action replaying as a sync mechanism.

This is different since your synchronization is actually a loose synchronization. In turn, it has 2 consequences:

To sum it up:

So the main is not a single source of truth, but simply a redux-aware process that you usually use to handle async data loading.

Better part: why use the main? actually we could use a third process, so that both the main process and the renderer process are free to go.

This way you can avoid to perform time consuming async action may lead to performance issue if you put them in the main.

I think that if this lib is to evolve, it could be in a direction that helps to spawn stores that communicate altogether, so that we can create a distributed redux-everywhere architecture, a kind of lightweight redux message queue. Also, using this logic, I think that maybe actions should not forwarded by default, we should have to add an additional meta to tel that its a forwarded action, and to whome it should go (all, main, or whatever named store).

We would not need aliasedAction anymore also, because if only the ith process have the relevant reducers/sagas/whatever, it will be the only one to process it.

RXminuS commented 7 years ago

Yeah this confused me at first as well especially since that's what https://github.com/samiskin/redux-electron-store does. I actually prefer this method because it means I can still use Immutable.js for the actual state in my renderers which helps with ensuring performance.

hardchor commented 7 years ago

Hey @eric-burel,

Thanks so much for your thoughts, and apologies for replying so late.

you can't guarantee the actual synchronization between stores when actions are missed (nobody cares because you should not miss actions in a normal electron use case but still, interprocess commmunication may theoritically fail)

You are absolutely correct. In addition, the initial architecture of electron-redux did not accommodate for local actions (which were added later), getting us somewhat further away from the single-source-of-truth statement.

I am not sure about that actually. That would be true if this lib was based on state diffing, but here, as far as I understand, you use action replaying as a sync mechanism.

Even with state-diffing, you could lose information through IPC. Last time I checked, https://github.com/samiskin/redux-electron-store uses some sort of state synchronisation and state filtering (as opposed to action replaying), which is why I created this library.

So the main is not a single source of truth, but simply a redux-aware process that you usually use to handle async data loading.

That is also correct, and should probably get updated in the docs.

Better part: why use the main? actually we could use a third process, so that both the main process and the renderer process are free to go.

The main process being the central point for all (except for local) actions fits in very nicely with the electron architecture. Renderer processes cannot talk to each other directly, so the main process' store needs to be the central hub for all communication.

This way you can avoid to perform time consuming async action may lead to performance issue if you put them in the main.

In all honesty, I can't think of a scenario where an action(-producer) or reducer would block the main thread long enough for it to actually matter. Due to the nature of the event loop, async actions are non-blocking, and reducers are simple stateless functions. Adding an extra process (to become what is now the main store) would - imho - add more overhead than the performance penalties you've mentioned. However, if this becomes a problem in your specific use-case, this library should not stop you from working around this, which is leading onto the next point...

We would not need aliasedAction anymore also, because if only the ith process have the relevant reducers/sagas/whatever, it will be the only one to process it.

Bingo! aliasedAction was always a bit of a pain point, and having named stores that you can freely forward to would be a way better alternative. This would, however, require quite an API change. Maybe we should open a new issue with a 2.0 roadmap.

Long story short:

I'm more than happy to evolve the library, and this is a great starting point. I'd still prefer for the main store to take up this special role as an "action fan" (or at least some sort of other util that could be decoupled from the store, but lives on the main thread), but I'm open for a discussion around this.

eric-burel commented 2 years ago

Soooo I finally have a chance to try this:

1. Make action local as a default

Goal: adding electron-redux does nothing, until you explicitely start sharing actions. Gives a more intuitive control over the setup.

This first step leads to a cleaner architecture.

2. Actions are not ordered

Goal: fix the issue that breaks the guarantee of actions being ordered, since they have to go through async process to reach the main

Second step was fixing the "async" issue: currently when actions are dispatched to the main process, they are also eliminated from the redux pipeline. Then the main process sends it back to the emitter with "local" scope.

The result is that shared actions behaves like "normal" actions (states update synchronously), their order is respected again.

Now that we have invoke method, starting with Electron 7, I also adopt the following pattern:

3. Create new renderer process with the same state as another one (or whatever state we want)

Goal: allow decoupling the main process state from renderers state

Final step: I'd like to be able to create new windows by duplicating the current window state. I think at the moment, when creating a new renderer, I use the main store => this forces the main store to be in sync with the renderer store. I'll change that, so that the main process store can have different data from the renderer store => this avoids duplication that can be problematic when the app grows big.

Code samples:

1.

For the action creator, I add "meta.scope.shared" to shared action + I mark them with the webcontent id

export function createSharedAction(
    // NOTE: Parameters doesn't work as expected with overloaded methods
    ...creatorArgs: /*Parameters<typeof createAction>*/ any
) {
    // @ts-ignore
    const baseActionCreator = createAction(...creatorArgs);
    function sharedActionCreator(...args) {
        const action = baseActionCreator(...args) as any;
        if (!action.meta) action.meta = {};
        action.meta.scope = 'shared';
        // in renderer process, mark with current id
        // /!\ this is very important, it avoid duplicating the event in the
        // emitter process
        if (typeof window !== 'undefined') {
            const electron = require('renderer/api/electron').default;
            action.meta.webContentsId = electron.getCurrentWebContentsId();
        }
        return action;
    }
    return sharedActionCreator;
}

Then in the renderer middleware. The change is that I call "next(action)": shared actions behaves like normal redux action from the renderer standpoint.

        // send to main process
        ipcRenderer.send('redux-action', action);
        // NOTE: it's important that the main process
        // do not send the action again to the emitter renderer
        // otherwise it would duplicate actions
        return next(action);

2.

And finally the main process. Here the difference is that I filter webContents, to avoid duplicating actions:

 // filter out the emitter renderer (important to avoid duplicate actions)
    let allWebContents = webContents.getAllWebContents();
    if (action?.meta?.webContentsId) {
        allWebContents = allWebContents.filter(
            (w) => w.id !== action.meta.webContentsId
        );
    }

    allWebContents.forEach((contents) => {
        contents.send('redux-action', rendererAction);
    });

    return next(action);

3.

When I create a new renderer process, I do this to pass the state explicitely:

        const newWindow = new BrowserWindow({
            show: false,
            width: 1024,
            height: 728,
            icon: iconPath,
            webPreferences: {
                // NOTE: this configuration is not secure! but removing nodeIntegration means reworking "app/renderer/api" files so they
                // don't rely directly on ipcRenderer etc. (including for 3rd party packages)
                nodeIntegration: true,
                nodeIntegrationInWorker: true,
                enableRemoteModule: true,

                preload: path.join(__dirname, './preload.js'),
                contextIsolation: false, // true // will be the default in Electron > 12
                // We also pass the initial state
                additionalArguments: [
                    `--redux-state=${initialReduxStateSerialized}`,
                ],
            },
        });

Then in my preload-source.js:

// Get initial redux state from args
// /!\ state must be serializable/deserializable
let initialReduxStateSerialized: string = '{}';
const reduxArg = window.process.argv.find((arg) =>
    arg.startsWith('--redux-state=')
);
if (reduxArg) {
    initialReduxStateSerialized = reduxArg.slice('--redux-state='.length);
}

And eventually:


export function getInitialStateRenderer() {
    try {
        const initialReduxState = JSON.parse(
            electron.initialReduxStateSerialized
        );
        return initialReduxState;
    } catch (err) {
        console.error(err);
        console.error('Serialized state', electron.initialReduxStateSerialized);
        throw new Error('Non serializable Redux state passed to new window');
    }

This comment is bit rushed out because I don't think there are enough users of Electron + Redux to make this a full Medium article or a full PR, but I'll be happy to answer any question if someone encounter the same issues.

For the record, this architeture would also work in a normal web app, with a worker playing the role of the main process.

eric-burel commented 2 years ago

One last thing that gives me a hard time is reloading the page. Since it erases all state in the renderer, it creates some discrepencies with the main state. Maybe I should setup a temporary system to memorize the renderer current state (eg in smth persistent like localStorage) and reload it from there.

Update: after more work on this, I think I need to totally decouple main and renderer state, and build the renderer so it can "rebuild" its state on reload by fetching relevant data

While right now, the main acts as a copy of the renderer process, which is not the right way to use it as it is difficult to guarantee synchronicity.