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

feat(StoreDevtools): implement actionsBlacklist/Whitelist & predicate #970

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

Wykks
Copy link

@Wykks Wykks commented Apr 14, 2018

Ref: #938

Note: the unit tests doesn't test jump / slider (I'm not sure how to test that).

There an issue when using the slider (and the action filter feature).
The app doesn't get the right state (whereas the extension show the correct state).
To reproduce in the example app, add actionsBlacklist:

    StoreDevtoolsModule.instrument({
      name: 'NgRx Book Store DevTools',
      logOnly: environment.production,
      actionsBlacklist: ['\\[Book\\] Search$']
    }),

Do two different search, then, jump or slide back to the previous Search Complete.

Sorry for leaving this issue here, I spend more than half a day figuring out how this works... This is as far as I'm able to do, for now... I'll try to investigate more If needed, but you probably already know what's going on 😅

Edit: Ok I see what's going on. Since I filter the liftedState only for the devtoolsExtension.send, the currentStateIndex send by the extension is desync with the internal liftedState.

I could actually filter-out the state outside extension.notify but according to: zalmoxisus/redux-devtools-extension#316 (comment) , the internal liftedState should not be filtered.

Edit 2 : Welp, I guess I should filter the state outside extension.notify after all. It works fine. But I'm a bit puzzled about how to "properly" code this. I'll update the PR next time (maybe tomorow).

@coveralls
Copy link

coveralls commented Apr 14, 2018

Coverage Status

Coverage decreased (-0.4%) to 88.385% when pulling 122a342 on Wykks:master into 49dcf7f on ngrx:master.

@dummdidumm
Copy link
Contributor

dummdidumm commented Apr 15, 2018

You could add a new property filteredOutActionIds to the reducer, which contains a list of action ids that should not appear on the monitor. That way, internally, the reducer keeps the filtered out actions and can properly recompute states. It also can use this array to compute an offset for the maxAge and for the "jump to state"-feature.
Inside extension.notify, the array is used to filter the actions/states and then pass it to the redux-devtools.

@brandonroberts
Copy link
Member

Thanks for the PR @Wykks! As an alternative, I'm looking into sending all actions/states through the extension since it already has support for blacklist/whitelist/predicate and I don't think we need to duplicate all that on our end.

@buenjybar
Copy link

👍 something like this will be nice !

@kevinahuber
Copy link

kevinahuber commented Jul 18, 2018

@Wykks Any word/work on this? Happy to help get this through.

@Wykks
Copy link
Author

Wykks commented Jul 20, 2018

Hi!

Sorry for this very long silence... I totally got demoralized doing this PR.
I can't help thinking that something is really wrong.

Just like Brandon said, this should be handled by the extension itself. But it doesn't work when sending the full state (with this.devtoolsExtension.send); because the extension doesn't filter anything in this case. That's why I started to handle both cases.

But, I really don't get why it's implemented like that in the extension. This should just be a visual thing, with some skip when moving through states. There's no actual need to filter the state like that...

@brandonroberts
Copy link
Member

@Wykks apologies for getting behind on this one. Will you rebase on master? Handling it here may be the option for now. We may need to open an issue on the extension to at least get some clarification on filtering when full state is provided.

@brandonroberts
Copy link
Member

@Wykks will you rebase this one?

@Wykks
Copy link
Author

Wykks commented Aug 8, 2018

ops didn't saw the last message. I can do the rebase this weekend yep!

@brandonroberts
Copy link
Member

👍

@Wykks
Copy link
Author

Wykks commented Aug 11, 2018

Rebased and updated!
Same thought on this stuff since the last time I spent time on it; it doesn't feel correct but it works.

@brandonroberts
Copy link
Member

@Wykks will you rebase again? Thanks

@Wykks
Copy link
Author

Wykks commented Sep 1, 2018

Done, sorry for the delay..

@brandonroberts brandonroberts merged commit 7ee46d2 into ngrx:master Sep 14, 2018
@brandonroberts
Copy link
Member

🎉

@wendelstam
Copy link

Really nice addition. In what version will this be included in the NPM package?

@timdeschryver
Copy link
Member

It will become available in the next version (7).
You can use it now by using the nightly builds.

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

Successfully merging this pull request may close these issues.

8 participants