microsoft / redux-dynamic-modules

Modularize Redux by dynamically loading reducers and middlewares.
https://redux-dynamic-modules.js.org
MIT License
1.07k stars 116 forks source link

Pass options to devtools #65

Closed anilanar closed 5 years ago

anilanar commented 5 years ago

Although it is convenient to include devtools integration in this library, there's a fair question to ask here: is it this library's responsibility to do it? If it's done for technical reasons, I think we need a way to pass options to the devtools' compose function.

Particularly action blacklisting, state sanitization, max-age are invaluable for large apps.

In addition, different apps have different needs (e.g. using devtools in production etc.). There are infinitely many possible ways to configure devtools. What if createStore took a compose function as a parameter?

Another alternative (as seen in other issues/questions submitted) is to have an additional API that exports enhancers/middlewares for dynamic module management and let users seeking more freedom use createStore from redux.

If you would like to go with the first option (adding compose param to createStore), I can make a PR. I suggest that createStore’s first argument is made an options object in that case e.g. createStore<...>(options: { compose, initialState, enchancersEtc }, ...modules). I’m typing from mobile so this is not a type signature :smile:

It seems it’d be better if this were done after https://github.com/Microsoft/redux-dynamic-modules/pull/55 is merged. Nevertheless this will be a breaking change.

anilanar commented 5 years ago

For now, I have a fork that makes createStore take compose function as an argument.

navneet-g commented 5 years ago

@anilanar Please feel free to send a PR.

navneet-g commented 5 years ago

@anilanar what if we create an option to createStore that tells it to not compose with devtools, and you can pass devtools as an enhancer in createStore?

anilanar commented 5 years ago

@navneet-g Devtools' compose function iterates all enhancers and generates an instanceId for each intermediate store created by enhancers and stores them somewhere:

https://github.com/zalmoxisus/redux-devtools-extension/blob/master/src/browser/extension/inject/pageScript.js#L383-L400

I don't understand why it needs to do that though. So createStore({}, [reduxDevtoolsCompose(/* enhancers */)]) would make devtools miss enhancers added by redux-dynamic-modules' own createStore.

dbartholomae commented 5 years ago

Any way I can help here? This is a major hindrance for us using this package now :-/

anilanar commented 5 years ago

I don’t see a way to make this a non-breaking change.

If maintainers are fine with a breaking change, and if they propose the new API, I can make a PR. Otherwise I don’t want to waste time on bikeshedding and on a PR.

navneet-g commented 5 years ago

@abettadapur what if we take a compose function as input? @dbartholomae @anilanar will that help?

dbartholomae commented 5 years ago

Taking a function sounds great! And if enhancers is a function instead of a middleware, dev-tools are not included by default. If this approach is fine, I could try to write a PR.

anilanar commented 5 years ago

I propose:

// The following plus 8 overloads for `DeepPartial<union of module states>`

interface StoreOptions<S> {
  initialState: S,
  enhancers?: StoreEnhancer[],
  extensions?: IExtension[],
  compose?: typeof reduxCompose,
  modules: IModule<any>[],
}

export declare function createStore<S>(options: StoreOptions<S>): IModuleStore<S>;

It might be possible to avoid overloads with some Typescript 3.0+ type-level wizardry: https://github.com/Microsoft/TypeScript/issues/26058#issuecomment-409772655

If someone's into it, it may be possible to take a tuple of modules like <M extends IModule<any>[]> and convert it to tuple of states with { [K in keyof M]: M[K] extends IModule<infer S> ? S : never } and then convert the tuple of states to intersection of states using the method provided in the link. Finally using initialState: DeepPartial<CalculatedState> and overloads are gone.

anilanar commented 5 years ago

I'm working on this now.

dbartholomae commented 5 years ago

Any news on the PR? Looking forward to this!

anilanar commented 5 years ago

Sorry I’ve been on a long vacation with no access to computers, will handle this when I’m back. On 23. Aug 2019, 04:40 +0300, Daniel Bartholomae notifications@github.com, wrote:

Any news on the PR? Looking forward to this!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/microsoft/redux-dynamic-modules/issues/65?email_source=notifications&email_token=AAFYASGQKHWPRAPWCKS4RSTQF45ZLA5CNFSM4G36CQF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD463WDI#issuecomment-524139277, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAFYASGZIN7TZ74TNESRPYDQF45ZLANCNFSM4G36CQFQ.