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

State / Action sanitizer refactoring/bugfix + options fix #855

Merged
merged 4 commits into from
Mar 5, 2018

Conversation

dummdidumm
Copy link
Contributor

@dummdidumm dummdidumm commented Feb 24, 2018

The new state / action sanitization feature added by #544 and #795 has a bug:

  1. Start up example app and open redux dev tools
  2. Open side drawer
  3. Export state (file A) -> you can already see that sanitizedActionsById and actionsById doesn't line up
  4. Import state (file A)
  5. Export again (file B)
  6. Import state (file B)
  7. Error

This may now be an edge case, but, if the persist feature would get implemented (which under the hood is basically export/import), it may occur very often.

This is because the redux devtools extension does not know about any sanitizedActionsById object and will not update that. But the ngrx devtools reducer relied on this getting updated. This PR moves the state / action sanitization out of the reducer into the DevtoolsExtension service. Now, the sanitization is done directly before passing the state updates to the redux devtools extension. This makes the code more clear and fixes that bug.

During this I noticed that the options object which is handed to the devtools extension on startup is different to the options object which is sent on a full state replacement. Fixed that, too.

No worries on the big added/removed lines count in the store.spec.ts btw, I only removed the now obsolete tests for sanitizedActionsById, but for some reason git made that into a "replaced the whole file"-commit.

@coveralls
Copy link

coveralls commented Feb 24, 2018

Coverage Status

Coverage increased (+0.7%) to 93.525% when pulling b2a8e5a on dummdidumm:master into 3b8d187 on ngrx:master.

}
}

private sanitizeAction(action: LiftedAction, actionIdx: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the sanitizer as an argument instead of checking its presence from the config inside the function.

: action;
}

private sanitizeStates(states: ComputedState[]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the sanitizer as an argument instead of checking its presence from the config inside the function.

}

private sanitizeState(state: any, stateIdx: number) {
return this.config.stateSanitizer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the sanitizer as an argument instead of checking its presence from the config inside the function.

}

private getExtensionConfig() {
const extensionOptions: ReduxDevtoolsExtensionConfig = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the config as an argument instead of using it inside the function

@brandonroberts
Copy link
Member

These changes look reasonable to me. I left some comments on minor tweaks. I would also consider moving the sanitization functions to the utils.ts file

@dummdidumm
Copy link
Contributor Author

Thanks for the feedback! I added a commit implementing your suggestions.

@brandonroberts brandonroberts merged commit a5dcdb1 into ngrx:master Mar 5, 2018
@brandonroberts
Copy link
Member

Thanks!

sandangel pushed a commit to sandangel/platform that referenced this pull request Mar 23, 2018
…grx#855)

* Also refactors state / action sanitization
* Improved consistency for config object provided to extension
* Moved action/state sanitization functions into utils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants