refactor(devtools): disable unsupported features by hawkgs · Pull Request #60585 · angular/angular · GitHub
Skip to content

refactor(devtools): disable unsupported features - #60585

Closed
hawkgs wants to merge 2 commits into
angular:mainfrom
hawkgs:devtools/unsupported-features
Closed

refactor(devtools): disable unsupported features#60585
hawkgs wants to merge 2 commits into
angular:mainfrom
hawkgs:devtools/unsupported-features

Conversation

@hawkgs

@hawkgs hawkgs commented Mar 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

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?

  • Yes
  • No

@ngbot ngbot Bot added this to the Backlog milestone Mar 27, 2025
@hawkgs

hawkgs commented Mar 27, 2025

Copy link
Copy Markdown
Member Author

@hawkgs
hawkgs force-pushed the devtools/unsupported-features branch from 32c78f3 to 4be0224 Compare March 27, 2025 15:43
dgp1130
dgp1130 reviewed Mar 27, 2025
View reviewed changes
Comment thread devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts Outdated
hawkgs
hawkgs commented Mar 27, 2025
View reviewed changes
Comment thread devtools/projects/protocol/src/lib/messages.ts Outdated
@hawkgs
hawkgs force-pushed the devtools/unsupported-features branch 4 times, most recently from 23a7a37 to 2b0b8bc Compare April 8, 2025 13:19
@hawkgs
hawkgs marked this pull request as ready for review April 8, 2025 13:20
@hawkgs

hawkgs commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

@dgp1130, the PR is ready. All new changes are in the fixup commit. I had to do some code restructuring – mainly extracting getRoots in a separate file in order to avoid circular dependency between component-tree.ts and ng-debug-api.ts. Added some tests and clarification comments.

dgp1130
dgp1130 approved these changes Apr 9, 2025
View reviewed changes

@dgp1130 dgp1130 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread devtools/projects/ng-devtools-backend/src/lib/component-tree/get-roots.spec.ts Outdated
Comment thread devtools/projects/ng-devtools-backend/src/lib/component-tree/types.ts Outdated
Comment thread devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts Outdated
Comment thread devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts Outdated
Comment thread devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.ts Outdated

@dgp1130 dgp1130 Apr 9, 2025

Copy link
Copy Markdown
Contributor

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?

@hawkgs hawkgs Apr 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

@dgp1130 dgp1130 Apr 11, 2025

Copy link
Copy Markdown
Contributor

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.

Comment thread devtools/projects/ng-devtools-backend/src/lib/ng-debug-api/ng-debug-api.spec.ts Outdated
Comment thread devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts Outdated
hawkgs added 2 commits April 10, 2025 12:12
@hawkgs
refactor(devtools): disable unsupported features
c23fa18 
Prepare the app for Wiz & ACX and handle unsupported features by disabling their respective UI.
@hawkgs
fixup! refactor(devtools): disable unsupported features
92e8a9d
@hawkgs
hawkgs force-pushed the devtools/unsupported-features branch from 2b0b8bc to 92e8a9d Compare April 10, 2025 09:12
AleksanderBodurri
AleksanderBodurri approved these changes Apr 14, 2025
View reviewed changes

@AleksanderBodurri AleksanderBodurri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this @hawkgs 🙏

@dgp1130 dgp1130 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Apr 21, 2025
@dgp1130

dgp1130 commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

Switching to target: major as there seems to be a merge conflict on 19.2.x.

@pkozlowski-opensource

pkozlowski-opensource commented Apr 22, 2025

Copy link
Copy Markdown
Member

This PR was merged into the repository by commit cebb9d2.

The changes were merged into the following branches: main

@hawkgs
hawkgs deleted the devtools/unsupported-features branch April 23, 2025 07:55
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented May 24, 2025

Copy link
Copy Markdown

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators May 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dgp1130 dgp1130 dgp1130 approved these changes
@AleksanderBodurri AleksanderBodurri AleksanderBodurri approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: devtools target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@hawkgs @dgp1130 @pkozlowski-opensource @AleksanderBodurri