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(store-devtools) add autoPause option #2941

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

david-shortman
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Users cannot configure this property.

Closes #2722

What is the new behavior?

Adds option to automatically pause capturing actions and state changes when the redux devtools extension window is not open.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

The default is set to false as that is the current behavior.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 20, 2021

Preview docs changes for eb659a3 at https://previews.ngrx.io/pr2941-eb659a3c/

@david-shortman david-shortman force-pushed the enable-auto-pause branch 2 times, most recently from a7ef479 to d2d8d78 Compare February 20, 2021 07:18
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

I don't think this will work because we still need to provide the option to the devtools.
We can do this by defining the autoPause property here and assign it's value here.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the changes!
In the future, could you push a separate commit with your changes please? This makes it easier to review, otherwise we have to take a look at the whole PR again 😅.

I also added an additional question that I forgot to ask previously.

@@ -51,7 +51,9 @@ function addImportToNgModule(options: StoreDevtoolsOptions): Rule {
const [instrumentNgModuleImport] = addImportToModule(
source,
modulePath,
`StoreDevtoolsModule.instrument({ maxAge: ${options.maxAge}, logOnly: environment.production })`,
`StoreDevtoolsModule.instrument({ maxAge: ${options.maxAge}, autoPause: ${
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we want to add this in the schematics by default.
Personally, I think lesser options is better, and I feel that the maxAge and logOnly properties are the most important.
But this got me thinking, perhaps we could follow-up this PR by providing all of the devtools options, and only add them to the config when they deviate from the default values. I don't know if that's worth it tho, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think allowing pass-through of the options could be helpful, but I think it's also worth evaluating how each feature works and if there's anything special we should be doing to support them better.

For instance, I've realized on this PR that, even though auto-pausing is enabled on the devtools side of things, the ngrx-devtools module is still using CPU time by sending data to the not-listening extension. I'm going to look at if it's possible to retrieve the extension status and not bother sending the data when auto-pause is enabled.

@david-shortman
Copy link
Contributor Author

In the future, could you push a separate commit with your changes please? This makes it easier to review, otherwise we have to take a look at the whole PR again 😅.

I'll do that! Question from that- I noticed that, in order for PRs to be merged to master, they're supposed to be a single commit. Is that something I should just wait to do after getting approvals?

@timdeschryver
Copy link
Member

@david-shortman we can squash the commits into one commit when we approve and merge the PR.

@david-shortman
Copy link
Contributor Author

I've opened an issue with redux devtools to see what api they'd want for retrieving the extension status: reduxjs/redux-devtools#708.

Currently (as far as I can tell), there's no way to do so. This PR could still be useful for enabling auto-pause, even if the NgRx devtools will waste some CPU sending state over to a not-listening extension.

@timdeschryver
Copy link
Member

So my assumption is that new actions won't be visible in the devtools, but they will still be sent to the devtools extenion. Is that correct?

If it's a big deal we could also prevent that the action is sent to the devtools. Although I'm not sure of the impact it will have, and if it's really needed to do add this.

@brandonroberts
Copy link
Member

Do we have an update on this PR? If we're blocked on the redux extension, I think we should close this until that's resolved and revisit.

@david-shortman
Copy link
Contributor Author

Do we have an update on this PR? If we're blocked on the redux extension, I think we should close this until that's resolved and revisit.

Without empirical evidence, my assumption is that this PR is useful even though it would be nice to completely disable sending data to the extension when it is not open.

Allowing passthrough of these options at least prevents the redux devtools from having a performance impact on its side.

@david-shortman david-shortman marked this pull request as ready for review May 18, 2021 01:43
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

I think this is already a step in the right decision, for those who encounter problems while using the DevTools. If this issue persists, we can take further steps although I'm not a big fan if this would mean that we have to add some checks whether we need to keep the DevTools state in sync.

Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

LGTM

@brandonroberts brandonroberts merged commit 698bd29 into ngrx:master Jun 9, 2021
@brandonroberts
Copy link
Member

Thanks @david-shortman for pushing through as this one took a while.

@david-shortman david-shortman deleted the enable-auto-pause branch March 27, 2022 14:48
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.

Add support for autoPause option
4 participants