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 redux dev tool trace support (#3517) #3665

Merged
merged 10 commits into from
Nov 25, 2022
Merged

feat(store-devtools): add redux dev tool trace support (#3517) #3665

merged 10 commits into from
Nov 25, 2022

Conversation

zizifn
Copy link
Contributor

@zizifn zizifn commented Nov 15, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Add redux trace option support into dev tool, related to (#3517)

Below is the redux doc for trace option.
https://github.com/reduxjs/redux-devtools/blob/main/extension/docs/API/Arguments.md#trace

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

What is the current behavior?

No trace shown in the redux dev tools extension.
Closes #3517 #1868

What is the new behavior?

Trace shown in the redux dev tools extension if enable.

image

Does this PR introduce a breaking change?

[ ] Yes
[ X ] No

Other information

@netlify
Copy link

netlify bot commented Nov 15, 2022

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 2180fed
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/63785a3232467400081cedb5

@zizifn

This comment was marked as resolved.

@zizifn
Copy link
Contributor Author

zizifn commented Nov 15, 2022

@brandonroberts @timdeschryver I created PR related to #3517, could you help me review it. And this is my first PR, if you have any questions or comments, please let me know.

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.

Hi, thanks for creating this PR.
I do have two questions.

  1. I added the trace: true config to our example app (npm run example:start), but the traces weren't coming through. I will take a closer look at this later, but could you reverify if it works on your end please? (
    StoreDevtoolsModule.instrument({
    ).

image

  1. What's the reason why the tracelimit is sometimes set to 20?

@zizifn
Copy link
Contributor Author

zizifn commented Nov 18, 2022

Hi, thanks for creating this PR.
I do have two questions.

  1. I added the trace: true config to our example app (npm run example:start), but the traces weren't coming through. I will take a closer look at this later, but could you reverify if it works on your end please? (
    StoreDevtoolsModule.instrument({
    ).

image

  1. What's the reason why the tracelimit is sometimes set to 20?

Thanks for review.

  1. dev tool extension has little bug in there. Make sure not focus on trace tab in the beginning or switch to action tab then switch trace again.

  2. I only set to 20 in the spec test. The default is 10 according to the redux document.

@timdeschryver add GIF below, in my example, I set the trace limit to 50, a small number may not be able show the full stack trace..
redux-ngrx

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.

Thanks for the elaboration, I left a few comments.

modules/store-devtools/src/config.ts Outdated Show resolved Hide resolved
modules/store-devtools/src/config.ts Outdated Show resolved Hide resolved
modules/store-devtools/src/extension.ts Outdated Show resolved Hide resolved
modules/store-devtools/src/extension.ts Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/store-devtools/index.md Outdated Show resolved Hide resolved
zizifn and others added 5 commits November 19, 2022 12:04
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
@zizifn
Copy link
Contributor Author

zizifn commented Nov 19, 2022

Thanks for the elaboration, I left a few comments.

These comments make sense to me. And apply the suggested change. Thanks.

@zizifn
Copy link
Contributor Author

zizifn commented Nov 22, 2022

@timdeschryver anything I can do for getting one more reviewer?

@brandonroberts brandonroberts changed the title feat(devtool): add redux dev tool trace support (#3517) feat(store-devtools): add redux dev tool trace support (#3517) Nov 25, 2022
@brandonroberts brandonroberts merged commit 187802a into ngrx:master Nov 25, 2022
@brandonroberts
Copy link
Member

Thanks @zizifn!

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.

store-devtools trace tab not working
3 participants