-
Notifications
You must be signed in to change notification settings - Fork 26.5k
refactor(devtools): disable unsupported features #60585
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
Conversation
@dgp1130, @AleksanderBodurri , could you take a quick look at the PR and verify whether I am in the right direction and the change is in line with the current BE design? First time updating that part of the app. Thanks! TBD: The feature has to be tested. |
32c78f3
to
4be0224
Compare
devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts
Outdated
Show resolved
Hide resolved
23a7a37
to
2b0b8bc
Compare
@dgp1130, the PR is ready. All new changes are in the fixup commit. I had to do some code restructuring – mainly extracting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor suggestions, but overall I think this looks great! Thanks for pushing on this.
devtools/projects/ng-devtools-backend/src/lib/component-tree/get-roots.spec.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools-backend/src/lib/component-tree/types.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts
Outdated
Show resolved
Hide resolved
// Temporary solution. Convert to an eligible API when available. | ||
// https://github.com/angular/angular/pull/60585#discussion_r2017047132 | ||
// If there is a Wiz application, make Profiler API unavailable. | ||
return !getRoots() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: How does DevTools behave if there are multiple roots and one is Angular and another is not? Could we enable these tabs as long as any one app is Angular? Would the functionality in those tabs even work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering that for some time as well but since we don't formally support multiple applications yet and I am not entirely sure how the app will react, I decided to play it on the safe side and disable the features altogether. I can do a more thorough check sometime next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it's untreaded ground, I'm mainly curious to how well DevTools reacts to multiple roots from the same application. I feel like that should work, but I haven't tried it.
I'm ok to be more restrictive here and we can open it up when we get to proper multi-app support.
devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.spec.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts
Outdated
Show resolved
Hide resolved
Prepare the app for Wiz & ACX and handle unsupported features by disabling their respective UI.
2b0b8bc
to
92e8a9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @hawkgs 🙏
Switching to |
This PR was merged into the repository by commit cebb9d2. The changes were merged into the following branches: main |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the new behavior?
Prepare the app for Wiz & ACX and handle unsupported features by disabling their respective UI.
Does this PR introduce a breaking change?