ioof-holdings / redux-subspace

Build decoupled, componentized Redux apps with a single global store
https://ioof-holdings.github.io/redux-subspace/
BSD 3-Clause "New" or "Revised" License
312 stars 33 forks source link

Problem with middleware after upgrading to version 2.2.0 #71

Closed psamusev closed 6 years ago

psamusev commented 6 years ago

I upgraded to version of redux-subspace 2.2.0 and started catch an error

Subspace epic couldn't find the store. Make sure you've used createEpicMiddleware from redux-subspace-observable Error: Subspace epic couldn't find the store. Make sure you've used createEpicMiddleware from redux-subspace-observable

Previous one - 2.1.1 worked correct

My environment is

    "redux": "^3.7.2",
    "redux-freeze": "^0.1.5",
    "redux-logger": "^3.0.6",
    "redux-observable": "^0.17.0",
    "redux-subspace": "^2.1.1",
    "redux-subspace-observable": "^2.1.1",

Also I use Angular 5 and Angular redux

    "@angular/common": "5.2.1",
    "@angular/compiler": "5.2.1",
    "@angular/core": "5.2.1",
    "@angular/forms": "5.2.1",
    "@angular/http": "5.2.1",
    "@angular/platform-browser": "5.2.1",
    "@angular/platform-browser-dynamic": "5.2.1",
    "@angular/platform-server": "5.2.1",
    "@angular/router": "5.2.1",
    "@angular-redux/router": "^7.0.0",
    "@angular-redux/store": "^7.1.0",
mpeyper commented 6 years ago

Can you share your store setup, including imports?

mpeyper commented 6 years ago

Just to clarify, I'm trying to ensure you are using the createEpicMiddleware wrapper from the redux-subspace-observable package and not the default one from redux-observable.

The cancellable-counter example is still working with the new version, so I'm assuming it is just a setup issue and not a bug in the new version.

It used to be an option to use the default one, but now that is not possible (which I have just realised needs updating in the docs)

psamusev commented 6 years ago

@mpeyper I see the differences in subspaced.ts file Previously it was

return epic => (action$, store, dependencies) => {
        const subspacedStore = subspaceDecorator(store)

        const filteredAction$ = action$
            ::map((action) => subspacedStore.processAction(action, identity))
            ::filter(identity)

        epic(filteredAction$, subspacedStore, dependencies).subscribe(subspacedStore.dispatch)
        return empty()
    }

but now I need put parent store as well - https://github.com/ioof-holdings/redux-subspace/blob/master/packages/redux-subspace-observable/src/observable/subspaced.js#L19 Btw

Could you refer me to an example how to use it correct now with dependencies

psamusev commented 6 years ago

Just to clarify, I'm trying to ensure you are using the createEpicMiddleware wrapper from the redux-subspace-observable package and not the default one from redux-observable.

Yes

Example of store creation

createStore(
        reducer,
        initState,
        applyMiddleware(...middlewares)

Example of creation substore

// setting subspace store
        this.store = subspace((s: any) => s[namespace], namespace)(parentStore);
mpeyper commented 6 years ago

Could you refer me to an example how to use it correct now with dependencies

You should not have to do anything special with the dependencies yourself, as long as you use createEpicMiddleware from the redux-subspace-observable package, NOT the standard one from redux-observable.

import { createStore } from 'redux'
import { applyMiddleware } from 'redux-subspace'
import { createEpicMiddleware } from 'redux-subspace-observable' // important: note the package name

const epicMiddleware = createEpicMiddleware(rootEpic)

const store = createStore(
  reducer,
  initState,
  applyMiddleware(epicMiddleware /*, other middleware */)
)
psamusev commented 6 years ago

I used this example - https://redux-observable.js.org/docs/recipes/AddingNewEpicsAsynchronously.html - to add epic dynamically. May it cause a problem?

psamusev commented 6 years ago

the whole code of initialization and creation store

const middlewares: Array<Middleware> = [createEpicMiddleware(epics.rootEpic)];
    let composeEnhancers = compose;
    const reducer: Reducer<S> = reducers.rootReducer;

    // on production we will not enable the dev tool and logging
    if (typeof window !== 'undefined' && !environment.production) {
        if ((window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__) {
            composeEnhancers = (window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__({});
        }
        middlewares.push(freeze);
        middlewares.push(logger);
    }

    createStore(
        reducer,
        initState,
        composeEnhancers(applyMiddleware(...middlewares))
    );

createEpicMiddleware is used from 'redux-subspace-observable'

mpeyper commented 6 years ago

I used this example - https://redux-observable.js.org/docs/recipes/AddingNewEpicsAsynchronously.html - to add epic dynamically. May it cause a problem?

Yes, that may be breaking it. Try passing on the dependencies in the rootEpic from that example:

const rootEpic = (action$, store, dependencies) =>
  epic$.mergeMap(epic =>
    epic(action$, store, dependencies)
  );

For reference - https://redux-observable.js.org/docs/recipes/InjectingDependenciesIntoEpics.html#injecting-dependencies

psamusev commented 6 years ago

@mpeyper sorry it was an old example. The correct one is

class ReduxEpicsRegistry<A extends Action, S> {

    public readonly rootEpic: Epic<A, S, any>;
    private epics: BehaviorSubject<Epic<A, S, any>>;

    /**
     * Creates new epics container.
     */
    constructor() {
        this.epics = new BehaviorSubject(combineEpics());
        this.rootEpic = (action$, store, dependencies) =>
            this.epics.mergeMap(streamEpic => streamEpic(action$, store, dependencies));
    }

    /**
     * Adding the epics to pipe.
     * @param epic which should be added
     * @param dependencies optional epic dependencies
     */
    add<D>(epic: Epic<A, S, D>, dependencies?: D): void {
        this.epics.next((action: ActionsObservable<A>, store: MiddlewareAPI<S>) => {
            return epic(action, store, dependencies);
        });
    }
}
psamusev commented 6 years ago

And as I said because of https://github.com/ioof-holdings/redux-subspace/blob/master/packages/redux-subspace-observable/src/observable/subspaced.js#L19 I don't get parentStore

mpeyper commented 6 years ago

I think the issue is in this bit:

add<D>(epic: Epic<A, S, D>, dependencies?: D): void {
    this.epics.next((action: ActionsObservable<A>, store: MiddlewareAPI<S>) => {
        return epic(action, store, dependencies);
    });
}

It is allowing the add function to add dependencies, but it is not passing on any that were added when the middleware was created (which is where the parent store is stored).

Unfortunately, it isn't as easy as merging them like so (my typescript is not strong so I went just javascript):

add(epic, dependencies) {
    this.epics.next((action, store, rootDependencies) => {
        return epic(action, store, { ...rootDependencies, ...dependencies });
    });
}

This is because we have not allowed the property to be enumerable and uses a non-obvious key so even manually copying it would be difficult.

For the sake of testing, can you try:

add(epic, dependencies) {
    this.epics.next((action, store, rootDependencies) => {
        return epic(action, store, {
          ['@@redux-subspace/store']: rootDependencies['@@redux-subspace/store'],
          ...rootDependencies,
          ...dependencies
        });
    });
}

If you're not actually using the dependencies param of the add function, you could also just move it to pass on the dependencies from the middleware instead:

add(epic) {
    this.epics.next((action, store, dependencies) => {
        return epic(action, store, dependencies);
    });
}
mpeyper commented 6 years ago

Maybe I was a bit quick to jump the gun and say merging the two dependencies won't work?

https://codesandbox.io/s/rm7328nynn?module=/src/epic/ReduxEpicsRegistry.js

add(epic, dependencies) {
    this.epics.next((action, store, rootDependencies) => {
        return epic(action, store, { ...rootDependencies, ...dependencies });
    });
}
psamusev commented 6 years ago

Yes, it helped. Thanks a lot. Feel free to close the ticket