ngrx / store-devtools

Developer Tools for @ngrx/store
MIT License
325 stars 38 forks source link

ReduxDevTools problem on 3.0.0 #27

Closed emilianosantucci closed 7 years ago

emilianosantucci commented 8 years ago

After upgrading on @ngrx/store-devtools@3.0.0, the Redux DevTools no longer inspect state.

I've tried with @ngrx/store-log-monitor:3.0.0 and its fine.

ritox842 commented 8 years ago

same issue...

Sn3b commented 8 years ago

same here :(

ritox842 commented 8 years ago

Is there an estimate time of fixing this bug?

avoskresensky commented 7 years ago

Here is what needs to be done to fix all the problems: 1) A typo which makes ReduxDevtoolsExtension undiscoverable.

instrument.ts

export function _createReduxDevtoolsExtension() {`
  if (typeof window !== 'undefined' && (window as any).devToolsExtension) {`
    return (window as any).devtoolsExtension;`  // <- typo, should be devToolsExtension
  }

  return null;
}

2) A bug causing the redirection of lifted actions into the regular dispatch

extension.ts

    // Listen for unlifted actions
    const actions$ = applyOperators(changes$, [
      //[ filter, change => change.type === ExtensionActionTypes.DISPATCH ], // that's incorrect
      [ filter, change => change.type === ExtensionActionTypes.ACTION ],     // that's correct
      [ map, change => change.payload ]
    ]);

3) A bug preventing the ReduxDevtoolsExtension from being notified about the lifted state changes

extension.ts

  notify(action, state: LiftedState) {
    if (!this.devtoolsExtension /*|| action.type !== ActionTypes.PERFORM_ACTION*/) { // we should notify of everything
      return;
    }

    //this.devtoolsExtension.send(unliftAction(state), unliftState(state), false, this.instanceId); // this doesn't seem to be needed
    this.devtoolsExtension.send(null, state, false, this.instanceId);
  }
ritox842 commented 7 years ago

@avoskresensky Ive done what youve said but still no change..

tamasfoldi commented 7 years ago

for me it worked

Sn3b commented 7 years ago

@ritox842 do the changes in the transpiled js files instead of the typescript source, at least that what I had to do.

avoskresensky commented 7 years ago

4) And another bug preventing the dispatching of hand-created actions:

extension.ts

    // Listen for unlifted actions
    const actions$ = applyOperators(changes$, [
      [ filter, change => change.type === ExtensionActionTypes.DISPATCH ],
   // [ map, change => change.payload ] <- this doesn't work, the payload is a string
       [map, function (change) { return typeof change.payload === 'string' ? eval('(' + change.payload + ')') : change.payload; }]
    ]);
avoskresensky commented 7 years ago

@ritox842, as @Sn3b correctly said, you'll need to modify the JS files if you want to patch your node module in place after the installation. I'm providing the references to the original TS files to expedite the proper fix. I've no time to make a fork and issue a PR at the moment, so that'll have to be it for now.

MikeRyanDev commented 7 years ago

@avoskresensky Big thanks for tracking down the changes. ❤️ I know you don't have time to send me a PR but can you shoot me your email address so I can set you as the commit author on the patch?

MikeRyanDev commented 7 years ago

Fixed in v3.1.0. I'll make an official announcement a little later but as a bonus I've added the ability to apply the instrumentation conditionally if the extension is present. This will not work if you also want to use @ngrx/store-log-monitor.

Usage:

@NgModule({
  imports: [
    StoreDevtoolsModule.instrumentOnlyWithExtension()
  ]
})
export class AppModule { } 

instrumentOnlyWithExtension() is AOT friendly making it ready for production builds.

Big thanks again to @avoskresensky for fixing the extension interop.

ritox842 commented 7 years ago

I did changed the .js files.. Any way, Iv'e upgrade the devtools to 3.1.0 and now it works.

Many Thanks @avoskresensky @MikeRyan52 !!!!!

tamasfoldi commented 7 years ago

Thanks @avoskresensky and @MikeRyan52!

@MikeRyan52 if you have time maybe you should update the changelog, and let the others know about the instrumentOnlyWithExtension() feeture. Maybe not everyone reads commits and closed issues and it can be helpful.

emilianosantucci commented 7 years ago

Thank you @avoskresensky for patching and @MikeRyan52 for update release.

Great work, maybe next time I will help you on troubleshooting and resolution :D

r-park commented 7 years ago

Thanks @avoskresensky and @MikeRyan52 :+1: