Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redux - Devtools are broken in release 2.0.1 #28

Closed
winklerp opened this issue Nov 10, 2018 · 14 comments
Closed

Redux - Devtools are broken in release 2.0.1 #28

winklerp opened this issue Nov 10, 2018 · 14 comments
Assignees

Comments

@winklerp
Copy link

The new ModuleEnhancer is composing the enhancers without redux devtools by using the plain compose method of redux and so the store is not recognized in the tools.

Fix in ModuleEnhancer.ts:

let composeEnhancers = compose;

if (process.env.NODE_ENV === "development") {
composeEnhancers = window["REDUX_DEVTOOLS_EXTENSION_COMPOSE"] || compose;
}

@navneet-g navneet-g self-assigned this Nov 10, 2018
@navneet-g
Copy link
Contributor

Thanks @winklerp for reporting the issue, seems like both this one and #24 are related.
I can not apply the dev tools enhancer at ModuleEnhancer as the consumer can already use devtools while creating the store. Also unfortunately we can not apply the dev tools compose twice :(.

I am thinking that I should revert my change to create moduleEnhancer, and just add parameter to configureStore which accepts enhancers that I can compose. @geminiyellow will that satisfy your scenario?

@winklerp
Copy link
Author

Hi @navneet-g, I made a quick fix in my repo and added the dev tools composer in moduleEnhancer. This works for me. I think if somebody uses redux-dynamic-modules adding the dev tools compose twice shouldn't be much of an issue, because you have to adjust your setup (configure store, ...) anyway. Maybe we could pass in the composer to moduleEnhancer as a parameter (with default = regular composer)?

What didn't work was to compose moduleEnhancer in redux createstore with the dev tools composer. In that case my module reducers weren't called anymore.

Actually I like the change with moduleEnhancer because we now can use the standard redux createstore method. What seems harder to solve with moduleEnhancer in my opinion is the issue in #24 when passing combined rootreducers to redux createstore. In that case the chained reducers get messed up and undefined keys without reducers are in the store.

@abettadapur
Copy link
Collaborator

Can we assert against passing reducers if you have used moduleEnhancer? It's not ideal, but it seems like that should work.

I think we should try to make the enhancer concept work unless it really cannot

@winklerp
Copy link
Author

Passing root reducers is not really necessary when using moduleEnhancers because you can pass the reducers with initial modules. That's how I did and it works fine. Maybe just a little note somewhere in the docs.

I agree - the module enhancer concept is good and we should make it work. I like it because we can use the standard redux createStore method.

@navneet-g
Copy link
Contributor

@abettadapur @winklerp I also like moduleEnhancer concept that is why I made the breaking changes :), but it is giving us a lot more trouble then it is helping. I tried few things to make it work with redux dev tools and fixing the issue in #24 but not having any success. I am open to a Pull Request if you have any suggestions.

@winklerp
Copy link
Author

I just added these lines in moduleEnhancer.ts and redux devtools work fine:

let composeEnhancers = compose;

if (process.env.NODE_ENV === "development") {
   composeEnhancers = window["REDUX_DEVTOOLS_EXTENSION_COMPOSE"] || compose;
}

We could also pass in composer as optional parameter to moduleEnhancer. Default value = Redux composer. I would appreciate not remove module enhancer. I also see not big deal with issue #24 because as I wrote before passing rootreducers with the initial modules works.

@navneet-g
Copy link
Contributor

Thanks @winklerp ... I already tried that, if we allow consumers to use redux store, I want to function it out of the box. The above will cause three non standard practices

  1. Consumers cannot use root reducers, instead need to create aux module
  2. They need to pass an overridden compose, if they want to handle the dev mode
  3. They need to make sure that moduleEnhancer is on the left side of applyMiddleware if any.

It is going to be hard to explain and might cause bugs. I prefer removing moduleEnhancer and just exposing createStore, as now the interface is pretty clean and easy to understand.

It will be easier for us to test redux-dynamic-modules.

@abettadapur what you suggest.

@abettadapur
Copy link
Collaborator

@navneet-g
I see, the main issue is that we have to compose the additional middleware, correct?

I kind of agree now, maybe we should just go back to createStore from the ModuleStore, and allow for enhancers to be passed in correctly. Points 1 and 3 are kind of an issue

@geminiyellow
Copy link

woo, breaking change again,
so, now there is no root reducers, right?
if i want to use this lib, i have to rewrite my redux entrance.

@winklerp
Copy link
Author

@navneet-g
I agree: the interface of createStore is more clear and it works out of the box. Actually having a Redux-Dynamic-Modules createStore method is not a big deal, as long as all options for configuring the store are exposed.

@geminiyellow
For having rootreducers I created modules, which are initially loaded (passed to createStore method as initial modules).

@geminiyellow
Copy link

@winklerp yep, i knew i can put root reducers into a root module,
but that is what i want to ask, in old project,

if i want to use this lib, i have to rewrite my redux entrance.

not just put integral module in some place.

@ghost
Copy link

ghost commented Nov 12, 2018

@winklerp and @geminiyellow thanks for your patience, help and feedback while we get it sorted.
I hate breaking changes myself, and this time I guess I got too carried forward while pushing the mouldEnhancer without thinking through all fallouts from that. I apologize again for that.

In terms of @geminiyellow question about using it in old project, I hope there are not too many places where you have redux entrance and it is easier to use.

We have not pushed the update to npm yet. If you have any alternate implementation please share a PR and we can discuss.
We will increase the major version so you don't have to upgrade if you are happy with what you have.

@geminiyellow
Copy link

geminiyellow commented Nov 13, 2018

hi @navneetg-msft , i like the moduleEnhancer but i also know #28 (comment) and sorry i dont have a good idea how to solve it now.

PS:

1 point in @navneet-g 's comment,

They need to make sure that moduleEnhancer is on the left side of applyMiddleware if any.

i dont think that is the package should do, we just need to put the notify in docs.

and here:

https://github.com/Microsoft/redux-dynamic-modules/blob/1ee66ccfaca7a49e0fdb0bb93bffe8f002fb1f64/packages/redux-dynamic-modules/src/ModuleStore.ts#L43-L47

how to change the middleware enhancer order if i want to put it in the first of all.
no way, right. that is same They need to make sure that applyMiddleware is on the right side

@geminiyellow
Copy link

about this, They need to pass an overridden compose, if they want to handle the dev mode
why dont you create a options param?
i dont think the lib should do this too:

https://github.com/Microsoft/redux-dynamic-modules/blob/1ee66ccfaca7a49e0fdb0bb93bffe8f002fb1f64/packages/redux-dynamic-modules/src/ModuleStore.ts#L38-L40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants